llvm dev - Jul 2010 - [LLVMdev] [HEADSUP] Another attempt at CallInst operand rotation

Hi all,

I am almost ready for the last step with landing my long-standing patch.
I have converted (almost) all low-level interface users of CallInst to
respective high-level interfaces. What remains is a handful of hunks
to flip the switch.

But before I do the final commit I'd like to coerce all external users
to code against the high-level interface too. This will (almost, but
see below) give us static guarantees that out-of-tree code remains
functional across this transition.

Here is my attack plan:

I will fire two rounds,
- the first will catch all instances of CallInst::get/setOperand(0, ...)
   and suggest using get/setCalledValue (or getCalledFuntion).
- the second will make all low-level operand accessors private
   in CallInst, and thus give external clients the chance to use
   *ArgOperand* versions. This will be well-commented in the
   header, explaining the recommended way of accessing
   arguments.

At this point we will have caught 99% of all low-level clients out  
there.

What uncertainties will remain? I can think of two of them:

   o getOperandNo()
   o access via baseclass pointer

The former is a method on Value::use_iterator and I cannot see a way to
intercept it at compile-time.
The latter is always possible and does circumvent the above measures,
there is no remedy against it.

I plan to fire each of these two rounds with one week delay and monitor
the LLVM mailing lists while they are soaking.

After that I'll commit the actual operand rotation.

Last but not least, there will be some cleanup commits:

  - removing CallInst::ArgOffset,
  - fixing the 80-column violations I have introduced,
  - doxygenizing the new interfaces,
  - re-enabling the low-level interface again (possibly
    after 2.8 has brached?).

Well, that's it. I hope that this order of commits will keep
the pain at a bearable level for everyone.

I would be thankful for any comments/suggestions
regarding this plan.

Cheers,

	Gabor

Gabor Greif wrote:
> Hi all,
>
> I am almost ready for the last step with landing my long-standing patch.
> I have converted (almost) all low-level interface users of CallInst to
> respective high-level interfaces. What remains is a handful of hunks
> to flip the switch.
>
> But before I do the final commit I'd like to coerce all external users
> to code against the high-level interface too. This will (almost, but
> see below) give us static guarantees that out-of-tree code remains
> functional across this transition.
>
> Here is my attack plan:
>
> I will fire two rounds,
> - the first will catch all instances of CallInst::get/setOperand(0, ...)
>    and suggest using get/setCalledValue (or getCalledFuntion).
> - the second will make all low-level operand accessors private
>    in CallInst, and thus give external clients the chance to use
>    *ArgOperand* versions. This will be well-commented in the
>    header, explaining the recommended way of accessing
>    arguments.
>   
Stupid question: is making the getOperand() method of CallInst going to 
work?  For example, if I have the following code:

void
method (Instruction * I) {
    I->getOperand(2);
    ...
}

void method2 (CallInst * CI) {
    method (CI);
    ...
}

Will method() still work when you make CallInst::getOperand() private?

-- John T.
> At this point we will have caught 99% of all low-level clients out  
> there.
>
> What uncertainties will remain? I can think of two of them:
>
>    o getOperandNo()
>    o access via baseclass pointer
>
> The former is a method on Value::use_iterator and I cannot see a way to
> intercept it at compile-time.
> The latter is always possible and does circumvent the above measures,
> there is no remedy against it.
>
> I plan to fire each of these two rounds with one week delay and monitor
> the LLVM mailing lists while they are soaking.
>
> After that I'll commit the actual operand rotation.
>
> Last but not least, there will be some cleanup commits:
>
>   - removing CallInst::ArgOffset,
>   - fixing the 80-column violations I have introduced,
>   - doxygenizing the new interfaces,
>   - re-enabling the low-level interface again (possibly
>     after 2.8 has brached?).
>
> Well, that's it. I hope that this order of commits will keep
> the pain at a bearable level for everyone.
>
> I would be thankful for any comments/suggestions
> regarding this plan.
>
> Cheers,
>
> 	Gabor
>
>
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>

Am 30.06.2010 um 23:31 schrieb John Criswell:
>
> Stupid question: is making the getOperand() method of CallInst  
> going to work?  For example, if I have the following code:
>
> void
> method (Instruction * I) {
>    I->getOperand(2);
>    ...
> }
>
> void method2 (CallInst * CI) {
>    method (CI);
>    ...
> }
>
> Will method() still work when you make CallInst::getOperand() private?
This is the case where I cannot help you (access via baseclass pointer).

For all these cases (there were a few in the llvm codebase too) I got
  - either assertion failures in the test suite
  - or obvious crashes
  - or miscompilations.

To catch all of these I could publish a patch (but not check it in)
that asserts that User::getOperand is not called on a CallInst.
But I can tell you that this will probably give you many false  
positives.

Btw. "method" above is of very little practical value, because
without knowing what type of instruction you have it makes
no sense to get its third operand. You will normally have a
dyn_cast<CallInst>(I) somewhere in "method".

Cheers,

	Gabor

>
> -- John T.
>
>> At this point we will have caught 99% of all low-level clients  
>> out  there.
>>
>> What uncertainties will remain? I can think of two of them:
>>
>>    o getOperandNo()
>>    o access via baseclass pointer
>>
>> The former is a method on Value::use_iterator and I cannot see a  
>> way to
>> intercept it at compile-time.
>> The latter is always possible and does circumvent the above measures,
>> there is no remedy against it.
>>
>> I plan to fire each of these two rounds with one week delay and  
>> monitor
>> the LLVM mailing lists while they are soaking.
>>
>> After that I'll commit the actual operand rotation.
>>
>> Last but not least, there will be some cleanup commits:
>>
>>   - removing CallInst::ArgOffset,
>>   - fixing the 80-column violations I have introduced,
>>   - doxygenizing the new interfaces,
>>   - re-enabling the low-level interface again (possibly
>>     after 2.8 has brached?).
>>
>> Well, that's it. I hope that this order of commits will keep
>> the pain at a bearable level for everyone.
>>
>> I would be thankful for any comments/suggestions
>> regarding this plan.
>>
>> Cheers,
>>
>> 	Gabor
>>
>>
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>>
>

Sounds great to me Gabor.  I really like your new incremental approach to this
patch set.

-Chris

On Jun 30, 2010, at 1:59 PM, Gabor Greif wrote:
> Hi all,
> 
> I am almost ready for the last step with landing my long-standing patch.
> I have converted (almost) all low-level interface users of CallInst to
> respective high-level interfaces. What remains is a handful of hunks
> to flip the switch.
> 
> But before I do the final commit I'd like to coerce all external users
> to code against the high-level interface too. This will (almost, but
> see below) give us static guarantees that out-of-tree code remains
> functional across this transition.
> 
> Here is my attack plan:
> 
> I will fire two rounds,
> - the first will catch all instances of CallInst::get/setOperand(0, ...)
>   and suggest using get/setCalledValue (or getCalledFuntion).
> - the second will make all low-level operand accessors private
>   in CallInst, and thus give external clients the chance to use
>   *ArgOperand* versions. This will be well-commented in the
>   header, explaining the recommended way of accessing
>   arguments.
> 
> At this point we will have caught 99% of all low-level clients out  
> there.
> 
> What uncertainties will remain? I can think of two of them:
> 
>   o getOperandNo()
>   o access via baseclass pointer
> 
> The former is a method on Value::use_iterator and I cannot see a way to
> intercept it at compile-time.
> The latter is always possible and does circumvent the above measures,
> there is no remedy against it.
> 
> I plan to fire each of these two rounds with one week delay and monitor
> the LLVM mailing lists while they are soaking.
> 
> After that I'll commit the actual operand rotation.
> 
> Last but not least, there will be some cleanup commits:
> 
>  - removing CallInst::ArgOffset,
>  - fixing the 80-column violations I have introduced,
>  - doxygenizing the new interfaces,
>  - re-enabling the low-level interface again (possibly
>    after 2.8 has brached?).
> 
> Well, that's it. I hope that this order of commits will keep
> the pain at a bearable level for everyone.
> 
> I would be thankful for any comments/suggestions
> regarding this plan.
> 
> Cheers,
> 
> 	Gabor
> 
> 
> 
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

Reminder...

Round one has been committed as
<http://llvm.org/viewvc/llvm-project?view=rev&revision=107432>

I hope that it got digested by now, as I plan to commit the second
round tomorrow.

In fact I made two test commits already:
r107480 and r107580, the former of which
actually uncovered some more uses of the
low-level interfaces in core LLVM that
have slipped through.

To be prepared, you can do a test run with
your external tree if you wish using the
latter revision:

svn up -r 107580  llvm

Please follow up this mail if you have worries or
encounter problems.

Cheers,

	Gabor




Am 01.07.2010 um 06:41 schrieb Chris Lattner:
> Sounds great to me Gabor.  I really like your new incremental  
> approach to this patch set.
>
> -Chris
>
> On Jun 30, 2010, at 1:59 PM, Gabor Greif wrote:
>
>> Hi all,
>>
>> I am almost ready for the last step with landing my long-standing  
>> patch.
>> I have converted (almost) all low-level interface users of  
>> CallInst to
>> respective high-level interfaces. What remains is a handful of hunks
>> to flip the switch.
>>
>> But before I do the final commit I'd like to coerce all external  
>> users
>> to code against the high-level interface too. This will (almost, but
>> see below) give us static guarantees that out-of-tree code remains
>> functional across this transition.
>>
>> Here is my attack plan:
>>
>> I will fire two rounds,
>> - the first will catch all instances of CallInst::get/setOperand 
>> (0, ...)
>>   and suggest using get/setCalledValue (or getCalledFuntion).
>> - the second will make all low-level operand accessors private
>>   in CallInst, and thus give external clients the chance to use
>>   *ArgOperand* versions. This will be well-commented in the
>>   header, explaining the recommended way of accessing
>>   arguments.
>>
>> At this point we will have caught 99% of all low-level clients out
>> there.
>>
>> What uncertainties will remain? I can think of two of them:
>>
>>   o getOperandNo()
>>   o access via baseclass pointer
>>
>> The former is a method on Value::use_iterator and I cannot see a  
>> way to
>> intercept it at compile-time.
>> The latter is always possible and does circumvent the above measures,
>> there is no remedy against it.
>>
>> I plan to fire each of these two rounds with one week delay and  
>> monitor
>> the LLVM mailing lists while they are soaking.
>>
>> After that I'll commit the actual operand rotation.
>>
>> Last but not least, there will be some cleanup commits:
>>
>>  - removing CallInst::ArgOffset,
>>  - fixing the 80-column violations I have introduced,
>>  - doxygenizing the new interfaces,
>>  - re-enabling the low-level interface again (possibly
>>    after 2.8 has brached?).
>>
>> Well, that's it. I hope that this order of commits will keep
>> the pain at a bearable level for everyone.
>>
>> I would be thankful for any comments/suggestions
>> regarding this plan.
>>
>> Cheers,
>>
>> 	Gabor
>>
>>
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>