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

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

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

-Chris

Gabor Greif wrote:

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".
  
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 wrote:

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

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?

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!

Cheers,

  Gabor

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

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

-eric