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

[Sotiris Apostolakis via Phabricator via llvm-commits]

apostolakis 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)))
----------------
davidxl wrote:
> Should 'ForSinking' be removed? The cost analysis and the actual sinking should be in sync.
I think 'ForSinking' should remain. There are instructions that we don't want to sink but could be expensive and add to a dependence chain that might render a conditional move too expensive. For example, a call instruction that might have side effects cannot be sunk, but converting the select to a branch could allow us to avoid blocking on the execution of this call instruction (even if it is on the common path and is scheduled for execution). Thus, it is better to consider the call instruction in the cost analysis.

Additionally, it just happens that one heuristic (base heuristic looking for expensive instructions) shares code with the sinking optimization. Other heuristics (e.g., loop-level, predictable branches) have different cost analyses. If a conversion is deemed profitable we always use the same sinking transformation logic regardless of the used heuristic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120233