[PATCH] D36753: [SimplifyCFG] Do not perform tail sinking if there are extra moves introduced

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1683
   Instruction *Cond = nullptr;
+  unsigned num_of_preds = 0;
   for (auto *B : predecessors(BBEnd)) {
----------------
Fix variable naming.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1725
+          // instruction.
+          if (InstructionsToSink.count(V) == 0 && isa<Constant>(V))
+            ++NumOfMovesForPHIdOperands;
----------------
Is the first check needed?


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1734
+
+  // If the number of moves is greater than the number of reuced instructions
+  // (which is InstructionsToSink.size() / 2), we bail out.
----------------
reuced --> reduced.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1735
+  // If the number of moves is greater than the number of reuced instructions
+  // (which is InstructionsToSink.size() / 2), we bail out.
+  // Do this only for BBend that has two unconditional predecessors and one
----------------
Should it be InstructionsToSink.size() -1?


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1738
+  // conditional predecessors. If there is only unconditional predecessors,
+  // there is chance to use select and generate code with using moves.
+  LRI.reset();
----------------
Explain a little more on the 'Select' part.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1741
+  unsigned NumOfMoves = CountNumberOfMovesForPHIOperands(LRI);
+  if (InstructionsToSink.size() > 0 && NumOfConditionsPreds == 1 &&
+      NumOfMoves >= InstructionsToSink.size() / 2) {
----------------
For O3, should we disable the sinking whenever there is runtime overhead introduced (unless there is runtime savings compensating it)?


================
Comment at: test/Transforms/SimplifyCFG/sink-common-code.ll:870
+return:
+  %retval.0 = phi i1 [ true, %if.then ], [ true, %if.then2 ], [ false, %if.else ]
+  ret i1 %retval.0
----------------
here the store can still be sunk -- while other instructions remain in their original location. This can slightly increase register pressure.


https://reviews.llvm.org/D36753