[PATCH] D13294: LEA code size optimization pass (Part 1): Remove redundant address recalculations

[Quentin Colombet via llvm-commits]

qcolombet added inline comments.

================
Comment at: lib/Target/X86/X86OptimizeLEAs.cpp:98
@@ +97,3 @@
+  const MachineBasicBlock *MBB = First.getParent();
+
+  // Both instructions must be in the same basic block.
----------------
aturetsk wrote:
> Should I put "\p" before every argument name mentioned in every comment?
Those are only proceeded in the doxygen like comments (e.g., ///).
In these comments, I think we usually put \p before every argument name.

================
Comment at: lib/Target/X86/X86OptimizeLEAs.cpp:105
@@ +104,3 @@
+         std::distance(MBB->begin(), MachineBasicBlock::const_iterator(&First));
+}
+
----------------
aturetsk wrote:
> > // 3) Displacement of the new memory operand should fit in 1 byte if possible.
> > // 4) The LEA should be as close to MI as possible, and prior to it if
> > //    possible.
> 
> These two conditions define the best LEA. Choosing 1 byte displacement over the bigger one leads to saving few bytes through different instruction encoding. And we try to use the closest LEA to avoid significant increasing of LEA's def liverange.
> And it just seems more right to me to give priority to the first LEA before MI than to the first LEA after MI regardless of the distance between LEAs and MI to avoid instruction moving.
Although I get what you are saying, it feels arbitrary to me to stop on the first LEA after the current instruction when this is the only candidate we found.
For now, this is fine, just make sure to add a FIXME.

================
Comment at: lib/Target/X86/X86OptimizeLEAs.cpp:133
@@ +132,3 @@
+      continue;
+
+    // Make sure address displacement fits 4 bytes.
----------------
aturetsk wrote:
> The case where this is needed is extremely rare. I used to have this in the initial version of the patch, but I wasn't able to write the test for it. So I just decided to drop it.
Maybe say that in the comment then.


http://reviews.llvm.org/D13294