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

[Sotiris Apostolakis via Phabricator via llvm-commits]

apostolakis marked an inline comment as done.
apostolakis added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectOptimize.cpp:99
   bool optimizeSelects(Function &F);
+  void optimizeSelectsBase(Function &F, SelectGroups &ProfSIGroups);
+  void optimizeSelectsInnerLoops(Function &F, SelectGroups &ProfSIGroups);
----------------
davidxl wrote:
> add some comments to the methods.
Done. 


================
Comment at: llvm/lib/CodeGen/SelectOptimize.cpp:352
+
+  // If unpredictable, branch form is more profitable.
+  if (SI->getMetadata(LLVMContext::MD_unpredictable)) {
----------------
davidxl wrote:
> is less profitable?
Yes. Fixed.


================
Comment at: llvm/lib/CodeGen/SelectOptimize.cpp:413
+
+void SelectOptimize::getOneUseSlice(Instruction *I,
+                                    SmallVector<Instruction *, 2> &Slice) {
----------------
davidxl wrote:
> This function deserves more comments in the code.
Added more comments.


================
Comment at: llvm/lib/CodeGen/SelectOptimize.cpp:444
+        continue;
+    }
+
----------------
davidxl wrote:
> should the relative hotness of II be checked instead of looking at the loop context?
> 
> Also It might worth check if the instructions in slice can actually be wrapped in the cold branch -- checking hasOneUse is for this, but is it more restricted?
That's a good idea. It is better to check the relative hotness rather than looking at loop contexts. Changed the code. Skipping instructions less hot than the source instruction (the value operand of the select). 

Checking for hasOneUse is a bit conservative (e.g., an instruction could have two uses that are both meant for the computation of the cold value operand of the target select) but it is much simpler than accurately identifying all the instructions used solely for the computation of the select’s true/false value operands. I originally implemented the latter but the experimental results did not show improved performance and thus the extra implementation complexity was not justified. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120231