[PATCH] D32277: Replace slow LEA instructions in X86

Lama via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Apr 26 07:05:41 PDT 2017


lsaba added a comment.

In https://reviews.llvm.org/D32277#731728, @skatkov wrote:

> Could you please also add some negative tests when you cannot do such transformation?
>  For example, involving eflags.


I added negative tests



================
Comment at: lib/Target/X86/X86FixupLEAs.cpp:110
+  /// Helper function for inserting a new instruction.
+  MachineBasicBlock::iterator Insert(MachineFunction::iterator MFI,
+                                     MachineBasicBlock::iterator Itr,
----------------
zvi wrote:
> Method name should start with a lowercase character
I removed this function


================
Comment at: lib/Target/X86/X86FixupLEAs.cpp:200
+MachineBasicBlock::iterator
+FixupLEAPass::Insert(MachineFunction::iterator MFI,
+                     MachineBasicBlock::iterator Itr, MachineInstr *NewMI) {
----------------
zvi wrote:
> Maybe pass a MachineBasicBlock &MBB instead?
I removed this function


================
Comment at: lib/Target/X86/X86FixupLEAs.cpp:201
+FixupLEAPass::Insert(MachineFunction::iterator MFI,
+                     MachineBasicBlock::iterator Itr, MachineInstr *NewMI) {
+  MFI->insert(Itr, NewMI);
----------------
zvi wrote:
> Itr -> InsertBefore
I removed this function


================
Comment at: lib/Target/X86/X86FixupLEAs.cpp:203
+  MFI->insert(Itr, NewMI);
+  DEBUG(NewMI->dump(););
+  return static_cast<MachineBasicBlock::iterator>(NewMI);
----------------
zvi wrote:
> What is the benefit of dumping every instruction added? If you do keep the print, please make it more descriptive.
> I know this was there before you moved the code, but still.
Keeping it since it's helpful, the description is at the beginning of the function


================
Comment at: lib/Target/X86/X86FixupLEAs.cpp:296
+static inline bool isInefficientLEAReg(unsigned int Reg) {
+  return Reg == X86::EBP || Reg == X86::RBP || Reg == X86::R13;
+}
----------------
efriedma wrote:
> Do you need to check for R13D?
there are no LEA instructions in 64bit which take 32-bit register operands, so we shouldn't run into this case


https://reviews.llvm.org/D32277





More information about the llvm-commits mailing list