[InstCombine] Re-enable guard canonicalization for AND and NOT OR

[mkazantsev]

Summary

The following changes:

r293061: "[InstCombine] Canonicalize guards for NOT OR condition"
r293058: "[InstCombine] Canonicalize guards for AND condition"

were reverted by the change

rL293227: "Revert a couple of InstCombine/Guard checkins"

because they contained a bug related to handling guards with multiple
arguments. The problem was that the extra arguments of the initial
guard were not passed to the newly-created guards.

This patch re-enables canonicalization of guards for AND and NOT OR
conditions and adds proper tests for them.

[mkazantsev]

ping

[apilipenko]

lib/Transforms/InstCombine/InstCombineCalls.cpp

3272–3279

SmallVector<Value *, 4> Args(CI->arg_begin(), CI->arg_end());
Args[0] = A;

And the same below.

[sanjoy]

As I said in the commit thread for rL293058, I don't think this is the right direction.

• This is almost the opposite of widening; running this after widening will undo widening.
• This increases the number of uses of deopt values, which will disable other instcombine transforms.
• I suspect we'll have to pull some heroics to not regress code size. This is addressable with some effort, but I'd rather spend that effort on improving passes that fall over today on guard(X & Y).

I think the canonical form be a maximally widened guard, and not the converse. I would instead look at passes that seem deficient in terms of analyzing widened guards, and fix them.

[mkazantsev]

I will prepare a couple of follow-up patches that widen the canonicalized guards after all transformations to avoid increase of code size due to this canonicalization.

lib/Transforms/InstCombine/InstCombineCalls.cpp

 
   case Intrinsic::experimental_guard: {
     Value *IIOperand = II->getArgOperand(0);
 
     // Remove a guard if it is immediately followed by an identical guard.
     if (match(II->getNextNode(),
               m_Intrinsic<Intrinsic::experimental_guard>(m_Specific(IIOperand))))
       return eraseInstFromFunction(*II);
+
+    // Canonicalize guard(a && b) -> guard(a); guard(b);
+    // Note: New guard intrinsics created here are registered by
+    // the InstCombineIRInserter object.
+    Function *GuardIntrinsic = II->getCalledFunction();
+    Value *A, *B;
+    OperandBundleDef DeoptOB(*II->getOperandBundle(LLVMContext::OB_deopt));
+    if (match(IIOperand, m_And(m_Value(A), m_Value(B)))) {
+      unsigned NumArgs = II->getNumArgOperands();
+      std::vector<Value*> Args;
+      Args.reserve(NumArgs);
+
+      // Prepare args for guard(a).
+      Args.push_back(A);
+      for (unsigned I = 1; I < NumArgs; ++I)
+        Args.push_back(II->getArgOperand(I));
+      CallInst *GuardA =
+          Builder->CreateCall(GuardIntrinsic, Args, {DeoptOB}, II->getName());
+
+      // Prepare args for guard(b).
+      Args[0] = B;
+      CallInst *GuardB =
+          Builder->CreateCall(GuardIntrinsic, Args, {DeoptOB}, II->getName());
+
+      auto CC = II->getCallingConv();
+      GuardA->setCallingConv(CC);
+      GuardB->setCallingConv(CC);
+      return eraseInstFromFunction(*II);
+    }
+
+    // guard(!(a || b)) -> guard(!a); guard(!b);
+    if (match(IIOperand, m_Not(m_Or(m_Value(A), m_Value(B))))) {
+      unsigned NumArgs = II->getNumArgOperands();
+      std::vector<Value*> Args;
+      Args.reserve(NumArgs);
+
+      // Prepare args for guard(!a).
+      Args.push_back(Builder->CreateNot(A));
+      for (unsigned I = 1; I < NumArgs; ++I)
+        Args.push_back(II->getArgOperand(I));
+      CallInst *GuardA =
+          Builder->CreateCall(GuardIntrinsic, Args, {DeoptOB}, II->getName());
+
+      // Prepare args for guard(!b).
+      Args[0] = Builder->CreateNot(B);
+      CallInst *GuardB =
+          Builder->CreateCall(GuardIntrinsic, Args, {DeoptOB}, II->getName());
+
+      auto CC = II->getCallingConv();
+      GuardA->setCallingConv(CC);
+      GuardB->setCallingConv(CC);
+      return eraseInstFromFunction(*II);
+    }
     break;
   }
   }
 
   return visitCallSite(II);
 }
 
 // InvokeInst simplification

test/Transforms/InstCombine/call-guard.ll

 ; CHECK-LABEL: @test_guard_adjacent_neg(
 ; CHECK-NEXT:    call void (i1, ...) @llvm.experimental.guard(i1 %A) [ "deopt"() ]
 ; CHECK-NEXT:    call void (i1, ...) @llvm.experimental.guard(i1 %B) [ "deopt"() ]
 ; CHECK-NEXT:    ret void
   call void(i1, ...) @llvm.experimental.guard( i1 %A )[ "deopt"() ]
   call void(i1, ...) @llvm.experimental.guard( i1 %B )[ "deopt"() ]
   ret void
 }
+
+define void @test_guard_and(i1 %A, i1 %B) {
+; CHECK-LABEL: @test_guard_and(
+; CHECK-NEXT:    call void (i1, ...) @llvm.experimental.guard(i1 %A, i32 123, i32 456, i32 789) [ "deopt"() ]
+; CHECK-NEXT:    call void (i1, ...) @llvm.experimental.guard(i1 %B, i32 123, i32 456, i32 789) [ "deopt"() ]
+; CHECK-NEXT:    ret void
+  %C = and i1 %A, %B
+  call void(i1, ...) @llvm.experimental.guard( i1 %C, i32 123, i32 456, i32 789 )[ "deopt"() ]
+  ret void
+}
+
+define void @test_guard_and_non_default_cc(i1 %A, i1 %B) {
+; CHECK-LABEL: @test_guard_and_non_default_cc(
+; CHECK-NEXT:    call cc99 void (i1, ...) @llvm.experimental.guard(i1 %A, i32 123, i32 456, i32 789) [ "deopt"() ]
+; CHECK-NEXT:    call cc99 void (i1, ...) @llvm.experimental.guard(i1 %B, i32 123, i32 456, i32 789) [ "deopt"() ]
+; CHECK-NEXT:    ret void
+  %C = and i1 %A, %B
+  call cc99 void(i1, ...) @llvm.experimental.guard( i1 %C, i32 123, i32 456, i32 789 )[ "deopt"() ]
+  ret void
+}
+
+define void @test_guard_not_or(i1 %A, i1 %B) {
+; CHECK-LABEL: @test_guard_not_or(
+; CHECK-NEXT:    %1 = xor i1 %A, true
+; CHECK-NEXT:    call void (i1, ...) @llvm.experimental.guard(i1 %1, i32 123, i32 456, i32 789) [ "deopt"() ]
+; CHECK-NEXT:    %2 = xor i1 %B, true
+; CHECK-NEXT:    call void (i1, ...) @llvm.experimental.guard(i1 %2, i32 123, i32 456, i32 789) [ "deopt"() ]
+; CHECK-NEXT:    ret void
+  %C = or i1 %A, %B
+  %D = xor i1 %C, true
+  call void(i1, ...) @llvm.experimental.guard( i1 %D, i32 123, i32 456, i32 789 )[ "deopt"() ]
+  ret void
+}
+
+define void @test_guard_not_or_non_default_cc(i1 %A, i1 %B) {
+; CHECK-LABEL: @test_guard_not_or_non_default_cc(
+; CHECK-NEXT:    %1 = xor i1 %A, true
+; CHECK-NEXT:    call cc99 void (i1, ...) @llvm.experimental.guard(i1 %1, i32 123, i32 456, i32 789) [ "deopt"() ]
+; CHECK-NEXT:    %2 = xor i1 %B, true
+; CHECK-NEXT:    call cc99 void (i1, ...) @llvm.experimental.guard(i1 %2, i32 123, i32 456, i32 789) [ "deopt"() ]
+; CHECK-NEXT:    ret void
+  %C = or i1 %A, %B
+  %D = xor i1 %C, true
+  call cc99 void(i1, ...) @llvm.experimental.guard( i1 %D, i32 123, i32 456, i32 789 )[ "deopt"() ]
+  ret void
+}