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

[Kai Luo via Phabricator via llvm-commits]

lkail marked 2 inline comments as done.
lkail 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();
+                }) &&
----------------
apostolakis wrote:
> 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.
Good catch. Added test for it.


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:6584
+         isSafeToSpeculativelyExecute(I) &&
+         (SinkCheapSelectOperand ||
+          TTI->getUserCost(I, TargetTransformInfo::TCK_SizeAndLatency) >=
----------------
apostolakis wrote:
> 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.
Good point! But I'm afraid CGP currently lacks facility exposing misprediction cost which should be defined by MCSchedModel. I happened to find X86 implements its own `X86CmovConversion.cpp` which uses more target-specific info to make the decision. The FIXME in `isFormingBranchFromSelectProfitable` also says
```
  // FIXME: This should use the same heuristics as IfConversion to determine
  // whether a select is better represented as a branch.
```
I starts doubting is it appropriate to get the task done in CGP since we don't have accurate target info.


================
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 =
----------------
apostolakis wrote:
> 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.
Yes, SimplifyCFG keeps all `!prof` metadata. What I meant in the summary is SimplifyCFG doesn't maintain branch probability which is calculated by BPI statically.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113872