[PATCH] D23911: [SimplifyCFG] Change the algorithm in SinkThenElseCodeToEnd

[Hans Wennborg via llvm-commits]

hans added inline comments.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1351
@@ +1350,3 @@
+  // stores, check the only user of each is a PHI or in the same block as the
+  // instruction, because if if a user is in the same block as an instruction
+  // we're contemplating sinking, it must already be determined to be sinkable.
----------------
"if if"

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1369
@@ -1431,3 +1368,3 @@
     };
     if (!all_of(Insts, SameAsI0)) {
       if (I0->mustOperandBeConstant(OI))
----------------
I'm still wondering how this code knows that I0 and I have the same number of operands. It looks like it assumed this before too. What am I missing?

The situation I was thinking off was calls with different number of arguments. Are we just getting lucky because of the "Don't create indirect calls" below?

If we assume the instructions have the same number of arguments, we should assert it, and otherwise we should bail. I suppose.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1489
@@ +1488,3 @@
+  // each block upwards in lockstep. If the n'th instruction from the end of each
+  // block can be sunk, those instructions are added to ValuesToSink and we
+  // carry on. If we can sink an instruction but need to PHI-merge some operands
----------------
maybe rename ValuesToSink to InstructionsToSink?

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1496
@@ +1495,3 @@
+  DenseMap<Instruction*, SmallVector<Value*,4>> PHIOperands;
+  while (PopulateInsts(ScanIdx) &&
+         canSinkInstructions(Insts, PHIOperands)) {
----------------
calling PopulateInsts with an increasing index like this is O(n^2). Maybe it doesn't matter much in practice (since n is number of sinkable instructions, large n would be rare?)

Would it be very impractical to just do a single reverse iteration over all the blocks instead of starting over from the back each time?


Repository:
  rL LLVM

https://reviews.llvm.org/D23911