[PATCH] D32277: Replace slow LEA instructions in X86

[Zvi Rackover via Phabricator via llvm-commits]

zvi added inline comments.


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


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


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


================
Comment at: lib/Target/X86/X86FixupLEAs.cpp:203
+  MFI->insert(Itr, NewMI);
+  DEBUG(NewMI->dump(););
+  return static_cast<MachineBasicBlock::iterator>(NewMI);
----------------
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.


================
Comment at: lib/Target/X86/X86FixupLEAs.cpp:204
+  DEBUG(NewMI->dump(););
+  return static_cast<MachineBasicBlock::iterator>(NewMI);
+}
----------------
return NewMI


================
Comment at: lib/Target/X86/X86FixupLEAs.cpp:299
+
+static inline bool isRegOperand(MachineInstr &MI, int Index) {
+  return MI.getOperand(Index).isReg() &&
----------------
This function and others below can take const MachineInstr& arguments.


================
Comment at: lib/Target/X86/X86FixupLEAs.cpp:318
+static inline bool isThreeOperandsLEA(MachineInstr &MI) {
+  return (isLEA(MI.getOpcode()) &&
+		  isRegOperand(MI,1) &&
----------------
Does it make more sense to check if isLEA() before calling this function and document that MI is a LEA instruction?


================
Comment at: lib/Target/X86/X86FixupLEAs.cpp:367
+
+static inline int getMOVrrFromLEA(MachineInstr &MI) {
+  int movrr_opcode;
----------------
Can you avoid the need for this function by using TII::copyPhysReg?


================
Comment at: lib/Target/X86/X86FixupLEAs.cpp:569
+
+    MFI->erase(Itr);
+    return;
----------------
Is it safe to erase while iterating over instructions? Maybe erase the instructions after you visited all the interesting instructions?


================
Comment at: lib/Target/X86/X86FixupLEAs.cpp:578
+    I = Insert(MFI, Itr,
+               BuildMI(*MF, DL, TII->get(opcode))
+                   .add(Dst)
----------------
You don't need to create an instruction and then insert it using your Insert function. There is already a  MachineInstrBuilder::BuildMI overload that does that


https://reviews.llvm.org/D32277