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

[David Majnemer via llvm-commits]

majnemer added inline comments.

================
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++)
----------------
Should we assert that both `I` and `E` have at least `NumOperands`?

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

================
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)
----------------
`auto *E`


http://reviews.llvm.org/D18990