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

[Mingming Liu via Phabricator via llvm-commits]

mingmingl added inline comments.


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

For my understanding, is it because of lengthy function body and long argument list that `inline` hint doesn't make much sense here?


================
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.");
----------------
skan wrote:
> Could we remove these assert?  I think they're not useful.
> Could we remove these assert?  I think they're not useful.

Removed the second `assert(CmpValDefInstr.getNumOperands() == 4)`.

I wonder if the 1st assert (`assert(MRI->getVRegDef(CmpInstr.getOperand(0).getReg()) == &CmpValDefInstr)`) helps in the sense that 
1) the information asserted (def-use between this two instructions) are not explicitly mentioned in any comment
2) this information is important for the correctness

Nevertheless kept the 1st assert and added comment. Let me know what I miss. 


================
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)
----------------
skan wrote:
> Useless assert
Since the function body `getImm()` or `getReg()` has asserts, it makes sense to just rely on that to give enough debugging information.

So removed three asserts (on `MO3, MO2, MO1`)


================
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
+    //
----------------
skan wrote:
> The format of comments here seems weird?
thanks for the catch! Fixed it.


================
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.
----------------
skan wrote:
> "The optimizatin will be disasbled"
> I think this is not covered by your test yet?  
it makes sense to add a test.

Added `test_not_erased_when_sf_used` in `llvm/test/CodeGen/X86/peephole-test-after-add.mir`, to cover the behavior that `TEST64rr` is retained if SF bit is used.


================
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`.
----------------
skan wrote:
> skan wrote:
> > Why don't we peek at imm here? It's possible to set `NoSignFlag` to `false`.
> Never mind. I missed craig's comments
Yeah I was initially thinking of peeking at imm, and later (upon Craig's comment) realized there is another way to do this.


================
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,
----------------
skan wrote:
> Could you uniform the instruction name in your comments? You use `SUBREG_TO_REG` here while `subreg_to_reg` somewhere.
Unified the instruction name in this patch.

p.s. I realized this sequence of instructions should be better placed around line 1024, so merge it with the sequence of instructions right after `if (X86::isAND(VregDefInstr->getOpcode())) {`


================
Comment at: llvm/test/CodeGen/X86/peephole-test-after-add.mir:2
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -o - %s -mtriple=x86_64-unknown-linux-gnu --run-pass=peephole-opt | FileCheck %s
+
----------------
craig.topper wrote:
> skan wrote:
> > skan wrote:
> > > Minor suggestion `-o - %s` -> `< %s`
> > Never mind. I didn't realize that "< %s" does not work for "--run-pass".
> I think it doesn't work for .mir input because llc uses the extension of the file name is used to determine file type. If you use re-direction llc can't see the extension so just assumes .ll.
thanks for the discussion! I'll keep it the current way and reply first, before getting a better idea of the difference (between `-o - %s` and `%s` in this command) myself.


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

https://reviews.llvm.org/D124118