[PATCH] D113872: [CGP] Handle select instructions relying on the same condition

[Sotiris Apostolakis via Phabricator via llvm-commits]

apostolakis added inline comments.


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:6578-6582
+         all_of(I->users(),
+                [&](User *user) {
+                  return std::find(ASI.begin(), ASI.end(),
+                                   dyn_cast<SelectInst>(user)) != ASI.end();
+                }) &&
----------------
This is not sound. Checking for just one use was conservative but it ensured that the instruction can be sinked. If other select instructions use the same operand, the operand can be sinked only if it is always used on the same path (i.e., always true or false operand).  

Here is an example:
```
%x = ...
%y = ...
%a = select %cond, %x, %y
%b = select %cond, %y, %x
```
If converted to branch, %x and %y cannot be sinked since they are needed in both paths:
```
%x = ...
%y = ...
if (%cond)
   %a = %x
   %b = %y
else
   %a = %y
   %b = %x
```

So, you will need to change the check here to account for such scenarios.


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:6584
+         isSafeToSpeculativelyExecute(I) &&
+         (SinkCheapSelectOperand ||
+          TTI->getUserCost(I, TargetTransformInfo::TCK_SizeAndLatency) >=
----------------
Enabling this option would make conversion to branches very aggressive without enough information to make such a decision. Sure in some cases it might be profitable to convert even selects with cheap operands but surely not always. For example, if the branch mispredicts once in a while, then the misprediction cost could outweigh the cost of computing all the operands of the select. Having this check only for expensive operands raised a bit the bar of how much misprediction would be acceptable.


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:6602-6616
+  if (any_of(ASI, [&](SelectInst *SI) {
+        uint64_t TrueWeight, FalseWeight;
+        if (SI->extractProfMetadata(TrueWeight, FalseWeight)) {
+          uint64_t Max = std::max(TrueWeight, FalseWeight);
+          uint64_t Sum = TrueWeight + FalseWeight;
+          if (Sum != 0) {
+            auto Probability =
----------------
You mentioned in the description that SimplifyCFGPass sometimes does not maintain branch metadata. Have you seen cases where the metadata is preserved only for a subset of a group of selects? If so, this change is useful. The ideal solution would be to address the issue in SimplifyCFGPass and avoid here the extra compile time and complexity of looking over all the select instructions (and avoid cases where none of the selects have branch info). But for now, at least it is worth adding a comment that explains why you need to check all of them so that this change can be reverted if the SimplifyCFGPass becomes better at maintaining metadata.


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:6621-6622
   // If a branch is predictable, an out-of-order CPU can avoid blocking on its
   // comparison condition. If the compare has more than one use, there's
   // probably another cmov or setcc around, so it's not worth emitting a branch.
+  if (!Cmp || !all_of(Cmp->uses(), [&](const Use &use) {
----------------
This comment needs to be changed if the proposed change below goes through. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113872/new/

https://reviews.llvm.org/D113872