[LLVMdev] [HEADSUP] Another attempt at CallInst operand rotation

[Gabor Greif]

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
LLV...@cs.uiuc.edu http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

[John Criswell]

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.

[Gabor Greif]

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

[Chris Lattner]

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

-Chris

[John Criswell]

It's example code to illustrate my concern. It doesn't exist anywhere.
Perhaps a more realistic example would be:

void
method (Instruction * I) {

for (unsigned index = 0; index < I->getNumOperands(); ++index) {
do_something_with (I->getOperand(index));
}
}

For example, code that computes a static backwards slice might do
something like the above. It doesn't care about operand order or what
type of instruction it is dealing with; it just wants to get the
incoming operands and do something with them.

This seems like a reasonable thing to do. Will code like this work if
you make CallInst::getOperand() private? If not, does your patch remove
code like the above from the LLVM source tree, and with what code do you
replace it?

-- John T.

[Gabor Greif]

This code will continue to compile and work as expected.
I'll only trap problematic usage when calling through
CallInst pointer (or reference).

As long as you do not make assumptions in your code
about operand ordering *inside of CallInst* you will be
on the safe side with my upcoming changes. Your above
example is perfectly OK and won't need to be redone.

Hope this helps!

[Gabor Greif]

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

[Eric Christopher]

On Jul 5, 2010, at 1:19 PM, Gabor Greif wrote:

> 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.

Thanks, it's been working well so far. I've updated some additional
code based upon your patches.

-eric