Modify constraints in `llvm::canReplaceOperandWithVariable`

[aoli]

Summary

Instruction::Switch: only first operand can be set to a non-constant value.
Instruction::InsertValue both the first and the second operand can be set to a non-constant value.
Instruction::Alloca return true for non-static allocation.

[aoli]

Add more constraints.

[aoli]

Remove Alloca.

[efriedma]

alloca in particular is a little tricky. It's legal to hoist a constant operand (it doesn't change the semantics), but it's a bad idea: it turns a constant-size stack allocation into a variable-size stack allocation, which is much slower. Better to avoid messing with it. (This is similar to the way the current callers of canReplaceOperandWithVariable handle calls: it's technically legal, but not a good idea.)

Other changes look fine.

[aoli]

Add Instruction::Alloca back.

[efriedma]

It would be nice to have a testcase for the InsertValue change, since it has an impact on SimplifyCFG even without D34921.

[aoli]

Test sink common code for insertvalue.

[efriedma]

LGTM

[aoli]

As we metioned in https://reviews.llvm.org/D34921. Should canReplaceOperandWithVariable only return false if isStaticAlloca is true for Instruction::Alloca here?

[efriedma]

Yes, that makes sense. (Please add a testcase to sink-common-code.ll.)

[aoli]

canReplaceOperandWithVariable return true for non-static alloca instruction.

[aoli]

Hi, I changed the canReplaceOperandWithVariable but I did not add test for sink-common-code.ll. because I found in SimplifyCFG.cpp canSinkInstructions: there is a check:

for (auto *I : Insts) {
  // These instructions may change or break semantics if moved.
  if (isa<PHINode>(I) || I->isEHPad() || isa<AllocaInst>(I) ||
      I->getType()->isTokenTy())
    return false;

So it will always return false for AllocaInst.

For GVN sink, alloca instruction is also blacklisted in GVNSink::isInstructionBlacklisted.

So the logic in canReplaceOperandWithVariable will never be reached.

[efriedma]

That's fine. LGTM.

lib/Transforms/Utils/Local.cpp

case Instruction::Invoke:
     // Constant bundle operands may need to retain their constant-ness for
     // correctness.
     if (ImmutableCallSite(I).isBundleOperand(OpIdx))
       return false;
     return true;
   case Instruction::ShuffleVector:
     // Shufflevector masks are constant.
     return OpIdx != 2;
+  case Instruction::Switch:
   case Instruction::ExtractValue:
-  case Instruction::InsertValue:
     // All operands apart from the first are constant.
     return OpIdx == 0;
+  case Instruction::InsertValue:
+    // All operands apart from the first and the second are constant.
+    return OpIdx < 2;
   case Instruction::Alloca:
-    return false;
+    // Static allocas (constant size in the entry block) are handled by
+    // prologue/epilogue insertion so they're free anyway. We definitely don't
+    // want to make them non-constant.
+    return !dyn_cast<AllocaInst>(I)->isStaticAlloca();
   case Instruction::GetElementPtr:
     if (OpIdx == 0)
       return true;
     gep_type_iterator It = gep_type_begin(I);
     for (auto E = std::next(It, OpIdx); It != E; ++It)
       if (It.isStruct())
         return false;
     return true;
   }
 }

test/Transforms/SimplifyCFG/sink-common-code.ll

 }
 
 ; CHECK-LABEL: @test_operand_bundles(
 ; CHECK: left:
 ; CHECK-NEXT:   %val0 = call i32 @call_target() [ "deopt"(i32 10) ]
 ; CHECK: right:
 ; CHECK-NEXT:   %val1 = call i32 @call_target() [ "deopt"(i32 20) ]
 
+%T = type {i32, i32}
+
+define i32 @test_insertvalue(i1 zeroext %flag, %T %P) {
+entry:
+  br i1 %flag, label %if.then, label %if.else
+
+if.then:
+  %t1 = insertvalue %T %P, i32 0, 0
+  br label %if.end
+
+if.else:
+  %t2 = insertvalue %T %P, i32 1, 0
+  br label %if.end
+
+if.end:
+  %t = phi %T [%t1, %if.then], [%t2, %if.else]
+  ret i32 1
+}
+
+; CHECK-LABEL: @test_insertvalue
+; CHECK: select
+; CHECK: insertvalue
+; CHECK-NOT: insertvalue
+
 ; CHECK: ![[TBAA]] = !{![[TYPE:[0-9]]], ![[TYPE]], i64 0}
 ; CHECK: ![[TYPE]] = !{!"float", ![[TEXT:[0-9]]]}
 ; CHECK: ![[TEXT]] = !{!"an example type tree"}