[PATCH] D120231: [SelectOpti][3/5] Base Heuristics

[Sotiris Apostolakis via Phabricator via llvm-commits]

apostolakis added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectOptimize.cpp:179-181
+  // If even a predictable select is cheap, then a branch cannot be cheaper.
+  if (!TLI->isPredictableSelectExpensive())
+    return false;
----------------
davidxl wrote:
> bsmith wrote:
> > Is this the correct thing to check for here?
> > 
> > Even if a select is cheap, the true/false values feeding it may not be, and given the later change you have that can sink these values when converted to a branch, this check may cause us to miss some cases where this pass is still useful. (Although perhaps changing this should be deferred to the patch that does the sinking).
> This is a good point.  Perhaps the target specific behavior should be modeled using a cost value (SelectCost) instead of using the binary knob.
You are right that there are still cases where it is worth considering converting to a branch even for architectures with cheap selects. I left this check since it was used in CodeGenPrepare when optimizing selects and I wanted to avoid unexpected optimizations for non-x86 architectures. However, since this pass will be, at least initially, opt-in and this check is not always useful, I will remove it. It will allow for easier non-x86 testing of the pass. Actually, when I tested some small examples on AArch64, I had to comment out this check since this pass got skipped otherwise. 

The cost of the select is already taken into account in the loop-level heuristics.
For the base heuristics, the heuristic looking for the expensive cold operand applies regardless of the cost of the select.
Yet, for the heuristic that converts highly predictable selects to branches, this check still applies. If a predictable select is not expensive, then the high predictability of the select does not suffice on its own for conversion to a branch. So, I added this check for this heuristic.

By the way, @bsmith let me know if you do any perfomance testing of this pass on ARM architectures. I will be happy to incorporate feedback and refine this pass to profitably target both x86 and ARM archs. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120231