[PATCH] D120232: [SelectOpti][4/5] Loop Heuristics

[Sotiris Apostolakis via Phabricator via llvm-commits]

apostolakis added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectOptimize.cpp:466
+  //     to its predicated version. The cost is in terms of latency and the cost
+  //     of a select group is the cost of its most expensive select instruction
+  //     (assuming infinite resources and thus fully leveraging available ILP).
----------------
davidxl wrote:
> The second heuristic is similar to the first one -- they are related to latency. Does it make more sense to consider the total pressure on resources?
> 
> Also it will be helpful to document in which function herusitc i and ii) are implemented.
Yes both heuristics are related to latency. But the first one is at a loop-level and all select groups are considered together (if all remain selects or all converted to branches; would be too expensive to try all different combinations), while the second one examines each select group separately. There are cases where one particular select group is unprofitable (or unnecessary) to convert while all the other ones should be converted. The second check allows us to prevent a conversion on the unprofitable one or save some code size if there is no perf diff. 

I think considering total pressure on resources would be a separate patch that needs some exploration. 

Improved the comments to clarify and added some pointers as suggested. 



================
Comment at: llvm/lib/CodeGen/SelectOptimize.cpp:672
+
+  Scaled64 Diff[2] = {LoopCost[0].PredCost - LoopCost[0].NonPredCost,
+                      LoopCost[1].PredCost - LoopCost[1].NonPredCost};
----------------
davidxl wrote:
> Perhaps name Diff to Gain to indicate it is the gain from converting Pred to NonPred.
Good point. Changed it. 
I actually also added more info in the opti remarks for this part of the code to make tuning of all the thresholds easier. 


================
Comment at: llvm/test/CodeGen/X86/select-optimize.ll:215
+;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
+;; Test loop heuristic: loop-level critical-path analysis
+;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
----------------
davidxl wrote:
> Add more test cases to cover other heuristics like gradiant
Yep I should have more testing coverage. Added.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120232