[PATCH] D151848: [X86, Peephole] Enable FoldImmediate for X86

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

Looks good and I think should be good to go with the `getVRegDef` thing fixed.



================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:3887
+    const MachineRegisterInfo &MRI = MI.getParent()->getParent()->getRegInfo();
+    MovMI = MRI.getVRegDef(MovReg);
+  }
----------------
`getVRegDef` must only be used when the code is still in (Machine-)SSA form and we have a guaranteed that there is only one or zero definitions. This code however runs later during regalloc I believe? So probably need to use `getUniqueVRegDef` here.

The code also has to be prepared for potential `nullptr`s in case of multiple definitions or no definition (could be live-in to the function or an `undef` operand for example).


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:4873
+      break;
+  }
+  return NewOpcode;
----------------
When inspecting something like `grep "32ri\s*=" build/lib/Target/X86/X86GenInstrInfo.inc` or `grep "64rr\s*=" build/lib/Target/X86/X86GenInstrInfo.inc` I noticed a couple more that probably work?
* ADC32rr / ADC32ri  (also ADD64rr)
* ROL32rCL / ROL32ri
* ROR32rCL / ROR32ri
* RCL32rCL / RCL32ri
* RCR32rCL / RCR32ri
* SBB32rr / SBB32ri  (also SBB64rr)
* TEST32rr / TEST32ri  (also TEST64rr)


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:4951-4954
+        UseMI.addOperand(MachineOperand::CreateReg(X86::EFLAGS, /*isDef*/ true,
+                                                   /*isImp*/ true,
+                                                   /*isKill*/ false,
+                                                   /*isDead*/ true));
----------------
Use the `/*xxx=*/` style which is shown in LLVM coding standards. It also has the advantage that clang-tidy can check this style whether the names match the function declarations (though nobody has actually enabled that check in LLVM, but I have seen it work nicely in other projects...)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151848/new/

https://reviews.llvm.org/D151848