[PATCH] D18990: [InstCombine] Remove trivially empty va_start/va_end and va_copy/va_end ranges.

[Arnaud A. de Grandmaison via llvm-commits]

aadg added a comment.

Thanks for looking into it Renato !


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:911
@@ +910,3 @@
+  unsigned NumArgOperands = I.getNumArgOperands();
+  assert(E.getNumArgOperands() == NumArgOperands &&
+         "the intrinsics must have the same number of operands.");
----------------
rengolin wrote:
> this could be a return false short-cut, not necessarily an assert...
The original intent is to make this helper only usable in this specific case (same number of argument). Any other case would point to an error.

After giving it a second thought, I think it's OK to make this helper a bit more "generic" so that it can be used as well in the VisitVACopy. I will update the patch in that direction.

================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:943
@@ +942,3 @@
+
+Instruction *InstCombiner::visitVAStartInst(VAStartInst &I) {
+  removeTriviallyEmptyRange(I, Intrinsic::vastart, Intrinsic::vaend, *this);
----------------
rengolin wrote:
> Where is this used and what's the point of always returning the null pointer?
The InstCombiner is an InstVisitor, so the dispatch to the visit* method is done by the base InstVisitor class.

This is the semantics of the InstCombiner when the instruction actually visited has just been removed (and replaced with something else).

================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:950
@@ +949,3 @@
+  // We can not use the removeTriviallyEmptyRange helper function here
+  // because va_copy has an extra argument in its prototype.
+  BasicBlock::iterator BI(I), BE(I.getParent()->end());
----------------
rengolin wrote:
> This could be a boolean flag to check for argument equality.
As stated above, I think the prototypes of removeTriviallyEmptyRange & haveSameOperands should be adapted so that this method is reusing those helpers.


http://reviews.llvm.org/D18990