[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 a comment.

(I forgot to click "reply" to send out the inline comments... but going to do it now)

Thanks for all reviews and inputs!

Going to hold it for a while since other reviewers might have comments as well.



================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:995
+      (MRI->getVRegDef(CmpInstr.getOperand(0).getReg()) == &CmpValDefInstr) &&
+      "Caller guaranteed.");
+
----------------
skan wrote:
> What does the "Caller guranteed" mean here?  Does it say that `TEST64rr` is the user of `SUBREG_TO_REG`?
> Does it say that TEST64rr is the user of SUBREG_TO_REG?

Yes this is correct. I think this assertion might prevent inadvertent misuse of `findRedundantFlagInstr`, yet I agree it's probably premature at this point.

Let me know if it's more idiomatic to just remove this assertion.


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:970
 
+inline static bool
+findRedundantFlagInstr(MachineInstr &CmpInstr, MachineInstr &CmpValDefInstr,
----------------
skan wrote:
> mingmingl wrote:
> > 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?
> Yes.  In fact, I think it's useless to add `inline` to functions w/ internal linkage for non-peformance critical code. Compiler can decide whether it should be inlined.
> Compiler can decide whether it should be inlined.

This is true.


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

https://reviews.llvm.org/D124118