[PATCH] D13297: [CGP] transform select instructions into branches and sink expensive operands

[Philip Reames via llvm-commits]

reames added a comment.

Overall, I think this is the right direction.  I might consider enhancing it slightly by sinking operands of the select's operands as long as there's only a single use, but that's gravy.  It would be entirely reasonable to do that in a separate change.

Trying to do this at the MI layer would be more complicated, less in alignment with how similar issues are already handled, and doesn't seem to give any real advantage.

I would like to see some performance numbers before submission.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:3845
@@ +3844,3 @@
+/// is only used once, return it. Otherwise, return a nullptr.
+static Instruction *sinkSelectOperand(const TargetTransformInfo *TTI,
+                                      Value *V) {
----------------
This would seem better structured as a boolean predicate on the Value.  

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:3882
@@ +3881,3 @@
+
+  // If either operand of the select is expensive, we should form a branch.
+  if (sinkSelectOperand(TTI, SI->getTrueValue()) ||
----------------
clarify comment: expensive and only needed on one side of the select

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:3929
@@ +3928,3 @@
+  //   select.end:
+  //      %sel = phi i32 [ %c, %entry ], [ %d, %select.false ]
+  //
----------------
I'd suggest revising this example.  In principal, we might create if/else/end blocks and sink into both if and else blocks.  I'd suggest formulating it that way and then mentioning that we optimize away one of the blocks if we can.

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:3952
@@ +3951,3 @@
+                                   EndBlock->getParent(), EndBlock);
+    BranchInst *TrueBranch = BranchInst::Create(EndBlock, TrueBlock);
+    SinkTrue->moveBefore(TrueBranch);
----------------
auto would be fine here.

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:3963
@@ +3962,3 @@
+  // If there was nothing to sink, then arbitrarily choose the 'false' side
+  // for a new landing pad input for the PHI.
+  if (TrueBlock == FalseBlock) {
----------------
!landing pad.  That has a very specific meaning in LLVM which this is not.  "input value", or "incoming edge" would be fine.

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:3964
@@ +3963,3 @@
+  // for a new landing pad input for the PHI.
+  if (TrueBlock == FalseBlock) {
+    FalseBlock = BasicBlock::Create(SI->getContext(), "select.false",
----------------
I'd add an assert inside this that TrueBlock == EndBlock just to make it obvious.  

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:3973
@@ +3972,3 @@
+
+  // If we did not create a new block for one of the 'true' or 'false' paths
+  // of the condition, it means that path actually originates from the start
----------------
I think that if you start the true and false blocks as being StartBlock you can simplify away this code entirely?


http://reviews.llvm.org/D13297