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

[aadg]

Summary

When a va_start or va_copy is immediately followed by a va_end (ignoring
debug information or other start/end in between), then it is safe to
remove the pair. As this code shares some commonalities with the lifetime
markers, this has been factored to helper functions. In the va_copy
case a specific handling in needed because it does not have the same
prototype as the va_end.

This InstCombine pattern kicks-in 3 times when running the LLVM test
suite.

[rengolin]

lib/Transforms/InstCombine/InstCombineCalls.cpp

1049

this could be a return false short-cut, not necessarily an assert...

1081

Where is this used and what's the point of always returning the null pointer?

1088

This could be a boolean flag to check for argument equality.

[aadg]

Thanks for looking into it Renato !

lib/Transforms/InstCombine/InstCombineCalls.cpp

1049

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.

1081

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

1088

As stated above, I think the prototypes of removeTriviallyEmptyRange & haveSameOperands should be adapted so that this method is reusing those helpers.

[aadg]

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

Following the review, rework the helper functions to simplify the patch further.

[aadg]

Ping ?

[majnemer]

lib/Transforms/InstCombine/InstCombineCalls.cpp

1048–1049

Should we assert that both I and E have at least NumOperands?

1057–1058

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.

1065

auto *E

[aadg]

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

lib/Transforms/InstCombine/InstCombineCalls.cpp

1048–1049

Yes, it does not hurt to wear a safety belt.

1057–1058

I think the behavior is fine because we only care about the trivial case. I will update the comment accordingly.

1065

Will fix.

[aadg]

Address David's comment.

[majnemer]

LGTM

[aadg]

Committed @ r269033.

Thanks David !

lib/Transforms/InstCombine/InstCombineCalls.cpp

static bool simplifyX86MaskedStore(IntrinsicInst &II, InstCombiner &IC) {
 
   IC.Builder->CreateMaskedStore(Vec, PtrCast, 1, BoolMask);
 
   // 'Replace uses' doesn't work for stores. Erase the original masked store.
   IC.eraseInstFromFunction(II);
   return true;
 }
 
+// Returns true iff the 2 intrinsics have the same operands, limiting the
+// comparison to the first NumOperands.
+static bool haveSameOperands(const IntrinsicInst &I, const IntrinsicInst &E,
+                             unsigned NumOperands) {
+  assert(I.getNumArgOperands() >= NumOperands && "Not enough operands");
+  assert(E.getNumArgOperands() >= NumOperands && "Not enough operands");
+  for (unsigned i = 0; i < NumOperands; i++)
+    if (I.getArgOperand(i) != E.getArgOperand(i))
+      return false;
+  return true;
+}
+
+// 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). As this handles only the most trivial
+// cases, tracking the nesting level is not needed:
+//
+//   call @llvm.foo.start(i1 0) ; &I
+//   call @llvm.foo.start(i1 0)
+//   call @llvm.foo.end(i1 0) ; This one will not be skipped: it will be removed
+//   call @llvm.foo.end(i1 0)
+static bool removeTriviallyEmptyRange(IntrinsicInst &I, unsigned StartID,
+                                      unsigned EndID, InstCombiner &IC) {
+  assert(I.getIntrinsicID() == StartID &&
+         "Start intrinsic does not have expected ID");
+  BasicBlock::iterator BI(I), BE(I.getParent()->end());
+  for (++BI; BI != BE; ++BI) {
+    if (auto *E = dyn_cast<IntrinsicInst>(BI)) {
+      if (isa<DbgInfoIntrinsic>(E) || E->getIntrinsicID() == StartID)
+        continue;
+      if (E->getIntrinsicID() == EndID &&
+          haveSameOperands(I, *E, E->getNumArgOperands())) {
+        IC.eraseInstFromFunction(*E);
+        IC.eraseInstFromFunction(I);
+        return true;
+      }
+    }
+    break;
+  }
+
+  return false;
+}
+
+Instruction *InstCombiner::visitVAStartInst(VAStartInst &I) {
+  removeTriviallyEmptyRange(I, Intrinsic::vastart, Intrinsic::vaend, *this);
+  return nullptr;
+}
+
+Instruction *InstCombiner::visitVACopyInst(VACopyInst &I) {
+  removeTriviallyEmptyRange(I, Intrinsic::vacopy, Intrinsic::vaend, *this);
+  return nullptr;
+}
+
 /// CallInst simplification. This mostly only handles folding of intrinsic
 /// instructions. For normal calls, it allows visitCallSite to do the heavy
 /// lifting.
 Instruction *InstCombiner::visitCallInst(CallInst &CI) {
   auto Args = CI.arg_operands();
   if (Value *V = SimplifyCall(CI.getCalledValue(), Args.begin(), Args.end(), DL,
                               TLI, DT, AC))
     return replaceInstUsesWith(CI, V);
case Intrinsic::stackrestore: {
 
     // If the stack restore is in a return, resume, or unwind block and if there
     // are no allocas or calls between the restore and the return, nuke the
     // restore.
     if (!CannotRemove && (isa<ReturnInst>(TI) || isa<ResumeInst>(TI)))
       return eraseInstFromFunction(CI);
     break;
   }
-  case Intrinsic::lifetime_start: {
-    // Remove trivially empty lifetime_start/end ranges, i.e. a start
-    // immediately followed by an end (ignoring debuginfo or other
-    // lifetime markers in between).
-    BasicBlock::iterator BI = II->getIterator(), BE = II->getParent()->end();
-    for (++BI; BI != BE; ++BI) {
-      if (IntrinsicInst *LTE = dyn_cast<IntrinsicInst>(BI)) {
-        if (isa<DbgInfoIntrinsic>(LTE) ||
-            LTE->getIntrinsicID() == Intrinsic::lifetime_start)
-          continue;
-        if (LTE->getIntrinsicID() == Intrinsic::lifetime_end) {
-          if (II->getOperand(0) == LTE->getOperand(0) &&
-              II->getOperand(1) == LTE->getOperand(1)) {
-            eraseInstFromFunction(*LTE);
-            return eraseInstFromFunction(*II);
-          }
-          continue;
-        }
-      }
-      break;
-    }
+  case Intrinsic::lifetime_start:
+    if (removeTriviallyEmptyRange(*II, Intrinsic::lifetime_start,
+                                  Intrinsic::lifetime_end, *this))
+      return nullptr;
     break;
-  }
   case Intrinsic::assume: {
     Value *IIOperand = II->getArgOperand(0);
     // Remove an assume if it is immediately followed by an identical assume.
     if (match(II->getNextNode(),
               m_Intrinsic<Intrinsic::assume>(m_Specific(IIOperand))))
       return eraseInstFromFunction(CI);
 
     // Canonicalize assume(a && b) -> assume(a); assume(b);

lib/Transforms/InstCombine/InstCombineInternal.h

public:
   Instruction *visitSwitchInst(SwitchInst &SI);
   Instruction *visitReturnInst(ReturnInst &RI);
   Instruction *visitInsertValueInst(InsertValueInst &IV);
   Instruction *visitInsertElementInst(InsertElementInst &IE);
   Instruction *visitExtractElementInst(ExtractElementInst &EI);
   Instruction *visitShuffleVectorInst(ShuffleVectorInst &SVI);
   Instruction *visitExtractValueInst(ExtractValueInst &EV);
   Instruction *visitLandingPadInst(LandingPadInst &LI);
+  Instruction *visitVAStartInst(VAStartInst &I);
+  Instruction *visitVACopyInst(VACopyInst &I);
 
   // visitInstruction - Specify what to return for unhandled instructions...
   Instruction *visitInstruction(Instruction &I) { return nullptr; }
 
   // True when DB dominates all uses of DI execpt UI.
   // UI must be in the same block as DI.
   // The routine checks that the DI parent and DB are different.
   bool dominatesAllUses(const Instruction *DI, const Instruction *UI,

test/Transforms/InstCombine/vararg.ll

+; RUN: opt < %s -instcombine -S | FileCheck %s
+
+%struct.__va_list = type { i8*, i8*, i8*, i32, i32 }
+
+declare void @llvm.lifetime.start(i64, i8* nocapture)
+declare void @llvm.lifetime.end(i64, i8* nocapture)
+declare void @llvm.va_start(i8*)
+declare void @llvm.va_end(i8*)
+declare void @llvm.va_copy(i8*, i8*)
+
+define i32 @func(i8* nocapture readnone %fmt, ...) {
+; CHECK-LABEL: @func(
+; CHECK: entry:
+; CHECK-NEXT: ret i32 0
+entry:
+  %va0 = alloca %struct.__va_list, align 8
+  %va1 = alloca %struct.__va_list, align 8
+  %0 = bitcast %struct.__va_list* %va0 to i8*
+  %1 = bitcast %struct.__va_list* %va1 to i8*
+  call void @llvm.lifetime.start(i64 32, i8* %0)
+  call void @llvm.va_start(i8* %0)
+  call void @llvm.lifetime.start(i64 32, i8* %1)
+  call void @llvm.va_copy(i8* %1, i8* %0)
+  call void @llvm.va_end(i8* %1)
+  call void @llvm.lifetime.end(i64 32, i8* %1)
+  call void @llvm.va_end(i8* %0)
+  call void @llvm.lifetime.end(i64 32, i8* %0)
+  ret i32 0
+}
+