[PATCH] D13295: LEA code size optimization pass (Part 2): Remove redundant LEA instructions

Andrey Turetskiy via llvm-commits llvm-commits at lists.llvm.org
Wed Dec 23 03:04:40 PST 2015


aturetsk added a comment.

Hi Quentin,
Thanks for the review.
This part reduces code size on Spec2000 for additional 0.1%, making it total 0.3% at -Oz.
The performance impact I measured was negative, yet very close to zero. I didn't want to possibly hurt performance at any rate at -Os, so I enabled it only for -Oz and I intend to keep it so until I do some analysis.
I don't see any significant compile time impact on Spec 2000 or the test from https://llvm.org/bugs/show_bug.cgi?id=25843. The complexity of removeRedundantLEAs is something like O(n^2), so the compile time should not blow up for big basic blocks.


================
Comment at: lib/Target/X86/X86OptimizeLEAs.cpp:76
@@ +75,3 @@
+  bool isReplaceable(const MachineInstr &First, const MachineInstr &Last,
+                     int64_t &AddrDispShift);
+
----------------
qcolombet wrote:
> Please comment what AddrDispShift is and how it should be used by the caller.
Done.

================
Comment at: lib/Target/X86/X86OptimizeLEAs.cpp:201
@@ +200,3 @@
+// these requirements must be met:
+// 1) Addresses calculated by LEAs deffer only by displacement.
+// 2) Def registers of LEAs belong to the same class.
----------------
qcolombet wrote:
> differ?
Fixed.

================
Comment at: lib/Target/X86/X86OptimizeLEAs.cpp:228
@@ +227,3 @@
+         Last.getOperand(0).isDef() && Last.getOperand(0).isReg() &&
+         "The first LEA operand must be def register");
+  for (auto &MO : MRI->use_operands(Last.getOperand(0).getReg())) {
----------------
qcolombet wrote:
> This assert is useless.
> This is already enforced by isLEA, isn’t it?
> 
> Moreover, in the previous check (getOperand(0).getReg()), the compiler would have asserted if the operand was not a register.
Removed.

================
Comment at: lib/Target/X86/X86OptimizeLEAs.cpp:246
@@ +245,3 @@
+      return false;
+    for (unsigned i = 0; i < MI.getNumOperands(); i++)
+      if (i != (unsigned)(MemOpNo + X86::AddrBaseReg) &&
----------------
qcolombet wrote:
> Could you add a comment over that loop?
Done.

================
Comment at: lib/Target/X86/X86OptimizeLEAs.cpp:398
@@ +397,3 @@
+                UE = MRI->use_end();
+           UI != UE;) {
+        MachineOperand &MO = *UI++;
----------------
qcolombet wrote:
> The formatting looks funny to me, have you used clang-format?
Absolutely, that's clang-format's doing.

================
Comment at: lib/Target/X86/X86OptimizeLEAs.cpp:415
@@ +414,3 @@
+        else if (Op->isGlobal())
+          Op->setOffset(Op->getOffset() + AddrDispShift);
+      }
----------------
qcolombet wrote:
> else llvm_unreachable or assert?
Done.

================
Comment at: lib/Target/X86/X86OptimizeLEAs.cpp:426
@@ +425,3 @@
+      // freely remove it.
+      assert(MRI->use_empty(Last.getOperand(0).getReg()));
+      Last.eraseFromParent();
----------------
qcolombet wrote:
> Add a message in the assert.
Done.


http://reviews.llvm.org/D13295





More information about the llvm-commits mailing list