[PATCH] D124118: [Peephole-Opt] For one kind of test-after-add pattern, eliminates test if it's correct to do so.

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:986
+
+  assert((SUBREG_TO_REG_MI.getOpcode() == X86::SUBREG_TO_REG) &&
+         "Should be guaranteed by caller.");
----------------
SUBREG_TO_REG is only used for i32->i64 conversions. So the only TEST it could ever be is TEST64rr.


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:997
+
+  MachineOperand MO1 = SUBREG_TO_REG_MI.getOperand(1);
+
----------------
This is making a copy of the a MachineOperand. Can you use a const reference?


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:999
+
+  // FIXME: Handle the case that MO1.isCImm() is true if necessary.
+  assert((MO1.isImm()) && "MO1 expected to be immediate number");
----------------
It's not necessary.


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:1010
+
+  MachineOperand MO2 = SUBREG_TO_REG_MI.getOperand(2);
+
----------------
This is making a copy of the MachineOperand


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:1015
+
+  if (!MO2.isKill()) {
+    return false;
----------------
Drop curly braces


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:1020
+  // SUBREG_TO_REG_MI has the last use of the register value.
+  MachineInstr *vregDefInstr = MRI->getVRegDef(MO2.getReg());
+
----------------
Variable names should be capitalized


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:1058
+
+    MachineOperand OutputOperand = vregDefInstr->getOperand(0);
+    if (OutputOperand.isReg()) {
----------------
Another copy


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:4318
+          assert(DefInstr != nullptr);
+          // {AND, OR, XOR} will update SF and won't update OF.
+          NoSignFlag = false;
----------------
AND/OR/XOR set the OF flag to 0.


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:4319
+          // {AND, OR, XOR} will update SF and won't update OF.
+          NoSignFlag = false;
+          ClearsOverflowFlag = false;
----------------
If you look through SUBREG_TO_REG then the width changed. The TEST64rr would have calculated the sign flag from bit 63(which is 0 due to the SUBREG_TO_REG), but the AND32 calculated it from bit 31 which is unknown.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124118