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

[Quentin Colombet via llvm-commits]

qcolombet added a comment.

Hi Andrey,

Thanks for working on this.

Out of curiosity, what performance impact do you see?
I.e., compile time and code size improvements.

Couple more comments inlined.

Thanks,
-Quentin


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

================
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.
----------------
differ?

================
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())) {
----------------
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.

================
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) &&
----------------
Could you add a comment over that loop?

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

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

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


http://reviews.llvm.org/D13295