Add -instcombine-code-sinking option

[andrew.w.kaylor]

Can you update this review with a summary that describes the problem your are trying to fix by disabling instruction sinking here?

lib/Transforms/InstCombine/InstructionCombining.cpp

123 ↗

(On Diff #167678)

This could use a longer description and a comment block explaining why it's an option. I also don't think it needs to be hidden. And for consistency with similar options, I would name this variable "EnableCodeSinking".

[GBuella]

Adding constructor argument as well -- making it more similar to the ExpensiveCombines option.

[craig.topper]

I don't know if we want this passed through the constructor. ExpensiveCombines is there because its attached -O3. I think we should only do this when we have a use case for it.

[xbolva00]

Wouldn't it better to pull it out of instcombine into.. let say.. CodeSinkingPass?

[craig.topper]

In D52709#1263663, @xbolva00 wrote:

Wouldn't it better to pull it out of instcombine into.. let say.. CodeSinkingPass?

Absolutely. This code doesn't belong in InstCombine. This code isn't even the code we would use if we had a real sinking pass. This is a very simple sinking algorithm. It only moves code to an immediate successor and it has a bogus one use check in it. I think we should have a real sinking pass. lib/Transforms/Scalar/Sink.cpp is probably a good starting point, I believe its only used by one target today. I think adding a disable here is a good first step towards being able to experiment with a real sinking pass.

[efriedma]

lib/Transforms/Scalar/Sink.cpp is probably a good starting point

I did an experiment with this recently. It seems to be in a pretty good state already; it reduced codesize for an ARM codebase I was looking at by about by about 0.1%. Haven't done any careful benchmarking, though.

Adding the command-line flag seems like enough to experiment. Not sure adding a flag to createInstructionCombiningPass() is really productive; it won't be useful once we turn off sinking by default.

[GBuella]

Back to having only a CLI option, no constructor argument.

[efriedma]

LGTM

[GBuella]

In D52709#1251147, @andrew.w.kaylor wrote:

Can you update this review with a summary that describes the problem your are trying to fix by disabling instruction sinking here?

Hi, Intel Graphics Compiler compiles uses LLVM to optimize scalar IR code, which is vectorized later on. It is pretty difficult to explain the exact problems code sinking can create.

[• wuzish]

In D52709#1263679, @craig.topper wrote:

In D52709#1263663, @xbolva00 wrote:

Wouldn't it better to pull it out of instcombine into.. let say.. CodeSinkingPass?

Absolutely. This code doesn't belong in InstCombine. This code isn't even the code we would use if we had a real sinking pass. This is a very simple sinking algorithm. It only moves code to an immediate successor and it has a bogus one use check in it. I think we should have a real sinking pass. lib/Transforms/Scalar/Sink.cpp is probably a good starting point, I believe its only used by one target today. I think adding a disable here is a good first step towards being able to experiment with a real sinking pass.

I also have a question that why lib/Transforms/Scalar/Sink.cpp is not enabled as default for all targets to do sinking instead of other separate places. @craig.topper

llvm/trunk/lib/Transforms/InstCombine/InstructionCombining.cpp

 STATISTIC(NumSunkInst , "Number of instructions sunk");
 STATISTIC(NumExpand,    "Number of expansions");
 STATISTIC(NumFactor   , "Number of factorizations");
 STATISTIC(NumReassoc  , "Number of reassociations");
 DEBUG_COUNTER(VisitCounter, "instcombine-visit",
               "Controls which instructions are visited");
 
 static cl::opt<bool>
+EnableCodeSinking("instcombine-code-sinking", cl::desc("Enable code sinking"),
+                                              cl::init(true));
+
+static cl::opt<bool>
 EnableExpensiveCombines("expensive-combines",
                         cl::desc("Enable expensive instruction combines"));
 
 static cl::opt<unsigned>
 MaxArraySize("instcombine-maxarray-size", cl::init(1024),
              cl::desc("Maximum array size considered when doing a combine"));
 
 // FIXME: Remove this flag when it is no longer necessary to convert
if (ExpensiveCombines && !I->use_empty() && Ty->isIntOrIntVectorTy()) {
         if (isInstructionTriviallyDead(I, &TLI))
           eraseInstFromFunction(*I);
         MadeIRChange = true;
         continue;
       }
     }
 
     // See if we can trivially sink this instruction to a successor basic block.
-    if (I->hasOneUse()) {
+    if (EnableCodeSinking && I->hasOneUse()) {
       BasicBlock *BB = I->getParent();
       Instruction *UserInst = cast<Instruction>(*I->user_begin());
       BasicBlock *UserParent;
 
       // Get the block the use occurs in.
       if (PHINode *PN = dyn_cast<PHINode>(UserInst))
         UserParent = PN->getIncomingBlock(*I->use_begin());
       else

llvm/trunk/test/Transforms/InstCombine/no_sink_instruction.ll

+; RUN: opt -instcombine -instcombine-code-sinking=0 -S < %s | FileCheck %s
+
+define i32 @test(i1 %C, i32 %A, i32 %B) {
+; CHECK-LABEL: @test(
+; CHECK: sdiv i32
+; CHECK-NEXT: add i32
+entry:
+        %tmp.2 = sdiv i32 %A, %B                ; <i32> [#uses=1]
+        %tmp.9 = add i32 %B, %A         ; <i32> [#uses=1]
+        br i1 %C, label %then, label %endif
+
+then:           ; preds = %entry
+; CHECK: ret i32
+        ret i32 %tmp.9
+
+endif:          ; preds = %entry
+; CHECK: ret i32
+        ret i32 %tmp.2
+}