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

[Arnaud A. de Grandmaison via llvm-commits]

aadg marked 3 inline comments as done.
aadg added a comment.

I will address your comment with an update to the patch.


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:913-914
@@ +912,4 @@
+// comparison to the first NumOperands.
+static bool haveSameOperands(const IntrinsicInst &I, const IntrinsicInst &E,
+                             unsigned NumOperands) {
+  for (unsigned i = 0; i < NumOperands; i++)
----------------
majnemer wrote:
> Should we assert that both `I` and `E` have at least `NumOperands`?
Yes, it does not hurt to wear a safety belt.

================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:922-923
@@ +921,4 @@
+// Remove trivially empty start/end intrinsic ranges, i.e. a start
+// immediately followed by an end (ignoring debuginfo or other
+// start/end intrinsics in between).
+static bool removeTriviallyEmptyRange(IntrinsicInst &I, unsigned StartID,
----------------
majnemer wrote:
> It seems to stop at the first end intrinsic which matches the start's operands, it doesn't necessarily skip _all_ end intrinsics.
> 
> If we have something like:
>   call @llvm.foo.start(i1 0)
>   call @llvm.foo.start(i1 0)
>   call @llvm.foo.end(i1 0) ; This will not be skipped, it will be removed
>   call @llvm.foo.end(i1 0)
> 
> That being said, this behavior is probably fine, I'd just like the comment to be a little more clear.
I think the behavior is fine because we only care about the trivial case. I will update the comment accordingly.

================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:930
@@ +929,3 @@
+  for (++BI; BI != BE; ++BI) {
+    if (IntrinsicInst *E = dyn_cast<IntrinsicInst>(BI)) {
+      if (isa<DbgInfoIntrinsic>(E) || E->getIntrinsicID() == StartID)
----------------
majnemer wrote:
> `auto *E`
Will fix.


http://reviews.llvm.org/D18990