[PM] Schedule InstSimplify after late LICM run, to clean up LCSSA nodes.

[mjacob]

Summary

The module pass pipeline includes a late LICM run after loop
unrolling. LCSSA is implicitly run as a pass dependency of LICM. However no
cleanup pass was run after this, so the LCSSA nodes ended in the optimized output.

[mehdi_amini]

lib/Transforms/IPO/PassManagerBuilder.cpp

547

InstCombine is expensive, do we have an alternative? What is the impact of not doing this cleanup here?

[mjacob]

Fix test.

[mjacob]

lib/Transforms/IPO/PassManagerBuilder.cpp

547

The LCSSA will end up in the optimized output. Two cases in which this was a problem for me:

1. If I run additional passes after O3, e.g. late statepoint insertion for GC, they get feed with suboptimal code, possibly affecting their output. That can of course easily be worked around by running InstCombine manually in between.

2. I'm working on a backend which directly emits code from LLVM IR. Coworkers complained about unnecessary phi nodes. Again this can be worked around by running InstCombine manually. However that possibly obfuscates the code when debugging.

I'm not sure whether this affects normal backends using SelectionDAG. At least I didn't find InstCombine in the llc pass pipeline.

[mjacob]

For reference: here is the bug report I filed recently: https://llvm.org/bugs/show_bug.cgi?id=27620

[majnemer]

lib/Transforms/IPO/PassManagerBuilder.cpp

547

Would InstSimplify work to get rid of these LCSSA PHI nodes?

[mjacob]

Run much cheaper InstSimplify instead of InstCombine.

[mjacob]

For reference: here is the bug report I filed recently: https://llvm.org/bugs/show_bug.cgi?id=27620

lib/Transforms/IPO/PassManagerBuilder.cpp

547

You're right. I was sure I tried this before.

[mjacob]

*ping*

Should be uncontroversial now, with InstSimplify instead of InstCombine.

[mehdi_amini]

test/Other/cleanup-lcssa.ll

15

Are we running such tests usually?
i.e. running something generic as "O3" and asserting the presence or absence of some instructions?

[mjacob]

test/Other/cleanup-lcssa.ll

15

I didn't find anything which was really comparable, but there are some tests that run opt -O3 .... test/Feature/OperandBundles/pr26510.ll even checks a whole function body.

[mehdi_amini]

LGTM.

lib/Transforms/IPO/PassManagerBuilder.cpp

if (!DisableUnrollLoops) {
     // LoopUnroll may generate some redundency to cleanup.
     addInstructionCombiningPass(MPM);
 
     // Runtime unrolling will introduce runtime check in loop prologue. If the
     // unrolled loop is a inner loop, then the prologue will be inside the
     // outer loop. LICM pass can help to promote the runtime check out if the
     // checked value is loop invariant.
     MPM.add(createLICMPass());
+
+    // Get rid of LCSSA nodes.
+    addInstructionCombiningPass(MPM);
   }
 
   // After vectorization and unrolling, assume intrinsics may tell us more
   // about pointer alignments.
   MPM.add(createAlignmentFromAssumptionsPass());
 
   if (!DisableUnitAtATime) {
     // FIXME: We shouldn't bother with this anymore.

test/Other/cleanup-lcssa.ll

+; RUN: opt -S -O3 %s | FileCheck %s
+
+define i64 @test() {
+entry:
+  br label %loop
+
+loop:
+  %i = phi i64 [ 0, %entry ], [ %inc, %loop ]
+  %inc = add i64 %i, 1
+  %cond = tail call i1 @check()
+  br i1 %cond, label %loop, label %exit
+
+exit:
+  ; CHECK-NOT: lcssa
+  ret i64 %i
+}
+
+declare i1 @check()