[PATCH] D35014: [X86] PR32755 : Improvement in CodeGen instruction selection for LEAs.

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added inline comments.


================
Comment at: lib/Target/X86/X86OptimizeLEAs.cpp:189
+static inline MemOpKey getMemOpCSEKey(const MachineInstr &MI, unsigned N) {
+  static MachineOperand DummyScale = MachineOperand::CreateImm(1);
+  assert((isLEA(MI) || MI.mayLoadOrStore()) &&
----------------
Can we avoid the static?


================
Comment at: lib/Target/X86/X86OptimizeLEAs.cpp:240
+
+class FactorizeLEAOpt {
+public:
----------------
Comment describing the purpose of the class


================
Comment at: lib/Target/X86/X86OptimizeLEAs.cpp:263
+
+   void addForLazyRemoval(MachineInstr * Instr) {
+      removedLEAs.push_back(Instr);
----------------
(style) cleanup the positions of the * - check what clang-format does


================
Comment at: lib/Target/X86/X86OptimizeLEAs.cpp:263
+
+   void addForLazyRemoval(MachineInstr * Instr) {
+      removedLEAs.push_back(Instr);
----------------
RKSimon wrote:
> (style) cleanup the positions of the * - check what clang-format does
comment


================
Comment at: lib/Target/X86/X86OptimizeLEAs.cpp:278
+
+   void insertLEA(MachineInstr * MI);
+
----------------
comment


================
Comment at: lib/Target/X86/X86OptimizeLEAs.cpp:298
+       insertLEA(&MI);
+   }
+}
----------------
(style) remove braces


================
Comment at: test/CodeGen/X86/lea-opt-csebb.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+
----------------
jbhateja wrote:
> lsaba wrote:
> > can you please add a test case that covers scale >1 cases 
> 
> This commit if you see has two parts
> 1/ pattern matching based on addressing mode (which is limited currently).
> 2/ factoring of LEAs which is generic.
> 
> Checking in incremental changes should be fine I guess.
> 
> Generic pattern will need to be brought out of addessing mode based selection as I described in following link 
> https://groups.google.com/forum/#!topic/llvm-dev/x2LDXpON500
> 
> Please comment in the thread. 
> 
Please can you commit this test file to trunk with current codegen and update the patch to show the diff


================
Comment at: test/CodeGen/X86/lea-opt-cst.ll:3
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
----------------
lsaba wrote:
> RKSimon wrote:
> > Drop the triple/datalayouts and just use the -mtriple=x86_64-unknown-linux-gnu instead.
> > 
> > Add a i686-unknown-linux-gnu test as well. 
> > 
> > By convention we tend to use X86 as the prefix for i686 triples and X64 for x86_64 triples.
> > 
> > You should be able to use utils/update_llc_test_checks.py to generate the codegen.
> please generate the test with the original checks before your changes and commit it first in a separate commit
Please can you commit this test file to trunk with current codegen and update the patch to show the diff


================
Comment at: test/CodeGen/X86/umul-with-overflow.ll:6
 define zeroext i1 @a(i32 %x)  nounwind {
+; CHECK-LABEL: a:
+; CHECK:       # BB#0:
----------------
lsaba wrote:
> jbhateja wrote:
> > lsaba wrote:
> > > why did this test change? 
> > Beecause I generated its output with script utils/update_llc_test_checks.py which adds 
> > an assertion for each instruction. I think it sould be fine. 
> This needs to be in a separate pre commit. please commit and rebase 
I regenerated this recently - please rebase


================
Comment at: utils/TableGen/DAGISelMatcherGen.cpp:308
   // If this is an 'and R, 1234' where the operation is AND/OR and the RHS is
-  // a constant without a predicate fn that has more that one bit set, handle
+  // a constant without a predicate fn that has more than one bit set, handle
   // this as a special case.  This is usually for targets that have special
----------------
craig.topper wrote:
> This should be a NFC pre-commit.
This is still here


https://reviews.llvm.org/D35014