[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:479
+MachineInstr *
+FixupLEAPass::processInstructionForSNBPlus(MachineBasicBlock::iterator &I,
+                                           MachineFunction::iterator MFI) {
----------------
No need to pass the iterator by reference. In fact, you could instead pass MachineInstr &MI and then you won't need to define 'MI' below.


================
Comment at: lib/Target/X86/X86FixupLEAs.cpp:483
+  MachineInstr &MI = *I;
+  const int opcode = MI.getOpcode();
+  if (!(isLEA(opcode) &&
----------------
Please capitalize


================
Comment at: lib/Target/X86/X86FixupLEAs.cpp:490
+
+  MachineOperand &Dst = MI.getOperand(0);
+  MachineOperand &Base = MI.getOperand(1);
----------------
This bunch of MachineOperands can all be const


================
Comment at: lib/Target/X86/X86FixupLEAs.cpp:514
+  const MCInstrDesc &ADDri = TII->get(getADDriFromLEA(MI));
+  MachineInstr *NewMI = nullptr;
+
----------------
You can avoid this early declaration and assignment to nullptr by defining variables local to the scope of their use.
At this point, it is known the function will never return nullptr, so the assignment here is not to a default return value.


================
Comment at: lib/Target/X86/X86FixupLEAs.cpp:596
+        if (MF.getSubtarget<X86Subtarget>().slow3OpsLEA()) {
+          if (auto *NewMI = processInstructionForSNBPlus(I, MFI)) {
+            MFI->erase(I);
----------------
Now that you added the feature, maybe rename 'processInstructionForSNBPlus' to something like 'processforSlow3OpLEA'?


================
Comment at: lib/Target/X86/X86Subtarget.h:253
 
+  /// True if the LEA instruction with 3 ops or certain registers is slow
+  bool Slow3OpsLEA;
----------------
Please specify in the comment all cases to which this feature applies.


https://reviews.llvm.org/D32277