instcombine pass before jumpthread for lifetimes (microsoft#4459)

Lifetime markers require a cleanup pass to eliminate some unnecessary
code, however one of those passes was JumpThreading, which only
eliminated certain dead blocks, which eliminated phi instructions
leaving looped self increments unconditionally incrementing themselves,
which ended up with infinite recursion when trying to process that
instruction.

By adding Instcombining first, all unreachable blocks are elimianted
first so the partial elimination doesn't leave such bad phi users. To
avoid losing convergence for DXIL specific operations, we must first run
the convergent marker pass, so these passes are moved a little later
than before.

Author: pow2clk

lib/Transforms/IPO/PassManagerBuilder.cpp

@@ -260,19 +260,19 @@ static void addHLSLPasses(bool HLSLHighLevel, unsigned OptLevel, bool OnlyWarnOn
   MPM.add(createDxilConditionalMem2RegPass(NoOpt));
   MPM.add(createDxilDeleteRedundantDebugValuesPass());
 
-  // Clean up inefficiencies that can cause unnecessary live values related to
-  // lifetime marker cleanup blocks. This is the earliest possible location
-  // without interfering with HLSL-specific lowering.
-  if (!NoOpt && EnableLifetimeMarkers) {
-    MPM.add(createSROAPass());
-    MPM.add(createJumpThreadingPass());
-  }
-
   // Remove unneeded dxbreak conditionals
   MPM.add(createCleanupDxBreakPass());
 
   if (!NoOpt) {
     MPM.add(createDxilConvergentMarkPass());
+    // Clean up inefficiencies that can cause unnecessary live values related to
+    // lifetime marker cleanup blocks. This is the earliest possible location
+    // without interfering with HLSL-specific lowering.
+    if (EnableLifetimeMarkers) {
+      MPM.add(createSROAPass());
+      MPM.add(createInstructionCombiningPass());
+      MPM.add(createJumpThreadingPass());
+    }
   }
 
   if (!NoOpt)

tools/clang/test/HLSLFileCheck/passes/llvm/instcombine/del_phi_self_reference.hlsl

@@ -0,0 +1,29 @@
+// RUN: %dxc -T ps_6_6 %s | FileCheck %s
+
+// Check a shader that previously resulted in a self-referencial add instruction
+// when the PHI it previously drew from was eliminated due to poor dead basic block elimination
+// The end result is a really dumb shader because most of the code is dead
+
+// CHECK: define void @main()
+// CHECK: call void @dx.op.storeOutput.f32(i32 5
+// CHECK-NEXT: ret void
+
+float main (uint2 param : P) : SV_Target {
+
+  bool yes = true;
+
+  // Trivially dead conditional block that becomes several basic blocks
+  // If BB elimination passes aren't done in the right order,
+  // Only some of the unused blocks could be destroyed and update the phi users incorrectly
+  if (!yes) {
+    for (int i = 0; i < 1; i++) {
+      // Force a PHI by breaking the block up
+      if (param.y)
+        break;
+      // This will use a PHI that, when the inital loop block is eliminated,
+      // could end up unconditionally adding to itself
+      ++param.x;
+    }
+  }
+  return 0;
+}