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

[Kan Shengchen via Phabricator via llvm-commits]

skan added a comment.

As to the desciption, maybe it should not be called a heuristic optimization?



================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:970
 
+inline static bool
+findRedundantFlagInstr(MachineInstr &CmpInstr, MachineInstr &CmpValDefInstr,
----------------
Remove `inline`?


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:988-993
+  assert(
+      (MRI->getVRegDef(CmpInstr.getOperand(0).getReg()) == &CmpValDefInstr) &&
+      "Caller guaranteed.");
+
+  assert((CmpValDefInstr.getNumOperands() == 4) &&
+         "Guaranteed by SUBREG_TO_REG definition.");
----------------
Could we remove these assert?  I think they're not useful.


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:1004
+  assert(MO3.isImm() && "MO3 is an immediate representing subregister indices");
+  // As seen in X86 td files, MO3 is typically sub_32bit or sub_xmm.
+  if (MO3.getImm() != X86::sub_32bit)
----------------
Useless assert


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:1026
+    // Get a sequence of instructions like
+    //   %reg = And32* %reg1 %reg2
+    //   ...                                                       // EFLAGS not
----------------
And32->and32


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:1027-1031
+    //   ...                                                       // EFLAGS not
+    //   changed %extended_reg = subreg_to_reg 0, %reg, %subreg.sub_32bit  //
+    //   EFLAGS not changed TEST64rr %extended_reg, %extended_reg, implicit-def
+    //   $eflags
+    //
----------------
The format of comments here seems weird?


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:1032
+    //
+    // The TEST64rr could be erased since And32* is known to set SF to zero.
+    for (MachineInstr &Instr :
----------------
And32->AND32


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:1048-1050
+    // However, the implementation artifically sets `NoSignFlag` to true
+    // to poison the SF bit; that is to say, if SF is looked at later, the
+    // optimization (to erase TEST64rr) will be disabled.
----------------
"The optimizatin will be disasbled"
I think this is not covered by your test yet?  


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:1054
+    // in the `AND` and `TEST` operation; signed bit is not known for `AND`
+    // without peeking at #imm, and is known to be 0 as a result of
+    // `TEST64rr`.
----------------
Why don't we peek at imm here? It's possible to set `NoSignFlag` to `false`.


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:1056-1058
+    // dest_reg = AND32ri8 src_reg, #imm
+    // extended_dest_reg = SUBREG_TO_REG 0, dest_reg, sub_32bit
+    // TEST64rr extended_dest_reg, extended_dest_reg,
----------------
Could you uniform the instruction name in your comments? You use `SUBREG_TO_REG` here while `subreg_to_reg` somewhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124118