[PATCH] D35014: [X86] PR32755 : Improvement in CodeGen instruction selection for LEAs.
Simon Pilgrim via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jul 26 04:24:54 PDT 2017
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
More information about the llvm-commits
mailing list