[PATCH] D120233: [SelectOpti][5/5] Optimize select-to-branch transformation

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectOptimize.cpp:732
+    // Avoid sinking other select instructions (should be handled separetely).
+    if (ForSinking && (II->isTerminator() || II->mayHaveSideEffects() ||
+                       isa<SelectInst>(II) || isa<PHINode>(II)))
----------------
Should 'ForSinking' be removed? The cost analysis and the actual sinking should be in sync.


================
Comment at: llvm/lib/CodeGen/SelectOptimize.cpp:398
+    // ILP with a naive ordering here.
+    SmallVector<Instruction *, 2> TrueSlicesInterleaved, FalseSlicesInterleaved;
+    for (unsigned long IS = 0; IS < maxTrueSliceLen; ++IS) {
----------------
apostolakis wrote:
> davidxl wrote:
> > Is it better to assert that TrueSlices and FalseSlices are not *both* empty? If that is that case, the transformation may be hurtful.
> Sinking is an added optimization on top of the benefit of breaking data dependences. 
> Even without any sinking, there are many cases where the transformation is still profitable.
> 
> For example, consider two dependence chains that include a load.
> critical path:             .... -> load -> select -> .....
> non-critical path                         \> store
> The first one is a long chain and it is the critical path while the second one is a much shorter one.
> The load cannot be sunk if the select is converted to a branch since it is used by another instruction (in the other path), but the conversion to a branch breaks a data dependence that reduces the critical path and thus it is still profitable. 
> 
Ok. I see that cost analysis already considers the 'sinkability' of the select operands in cost analysis.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120233