[PATCH] D98564: [AArch64] Peephole rule to remove redundant cmp after cset.

[Pavel Iliin via Phabricator via llvm-commits]

ilinpv marked 10 inline comments as done.
ilinpv added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:1641
+                 const TargetRegisterInfo *TRI, UsedNZCV *NzcvUse = nullptr,
+                 SmallVectorImpl<MachineInstr *> *CCUseInstrs = nullptr) {
+  if (MI->getParent() != CmpInstr->getParent())
----------------
paquette wrote:
> I think I would prefer an `Optional<ArrayRef<MachineInstr *>>` here if possible. I think that if someone were to use this and accidentally pass nullptr, it would be a mistake. Using an `Optional` better communicates that this is an optional parameter.
I am not sure about ArrayRef and const in examineCFlagsUse. The number of CCUseInstrs is defined in the analysis, instructions from CCUseInstrs can be later modified inverting condition code, CmpInstr can be removed.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:1665
+  if (NzcvUse)
+    NzcvUse = &NZCVUsedAfterCmp;
+  return (!NZCVUsedAfterCmp.C && !NZCVUsedAfterCmp.V);
----------------
paquette wrote:
> I don't think this actually does anything? It's writing to a local variable.
Goog catch!  Lost *  after code refactoring.  Test for that added.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:1742
+///
+/// CmpInstr can be removed if:
+/// - CmpInstr is 'ADDS %vreg, 0' or 'SUBS %vreg, 0',
----------------
paquette wrote:
> I don't think it's necessary to enumerate every case in this comment. It may fall out of sync if someone updates the code.
I was inspired by canInstrSubstituteCmpInstr doxygen comments. I guess it should be clear what function is doing.  Does it make sense to leave general  doxygen comment for canCmpInstrBeRemoved, but put detailed ordinary comments inside function plus doxygen comments for examineCFlagsUse?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98564