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

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:1535
 
+static int findCondCodeUseOperandIdx(const MachineInstr &Instr) {
+  switch (Instr.getOpcode()) {
----------------
- The name here is a little misleading. This is intended only for Bcc + select instructions. Maybe `findCondCodeUseOperandIdxForBranchOrSelect`?

- This could use a Doxygen comment describing what it does.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:1638
 
+static bool
+examineCFlagsUse(MachineInstr *MI, MachineInstr *CmpInstr,
----------------
This could use a doxygen comment documenting what the function is for.

It'd also be nice to document the parameters in the Doxygen comment as well.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:1638
 
+static bool
+examineCFlagsUse(MachineInstr *MI, MachineInstr *CmpInstr,
----------------
paquette wrote:
> This could use a doxygen comment documenting what the function is for.
> 
> It'd also be nice to document the parameters in the Doxygen comment as well.
If you return an `Optional` here you can still treat it like a bool, but also avoid passing `UsedNZCV` as an out-parameter.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:1639
+static bool
+examineCFlagsUse(MachineInstr *MI, MachineInstr *CmpInstr,
+                 const TargetRegisterInfo *TRI, UsedNZCV *NzcvUse = nullptr,
----------------
I don't think these should ever be nullptr?


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:1641
+                 const TargetRegisterInfo *TRI, UsedNZCV *NzcvUse = nullptr,
+                 SmallVectorImpl<MachineInstr *> *CCUseInstrs = nullptr) {
+  if (MI->getParent() != CmpInstr->getParent())
----------------
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.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:1651
+       instructionsWithoutDebug(std::next(CmpInstr->getIterator()),
+                                CmpInstr->getParent()->instr_end())) {
+    if (Instr.readsRegister(AArch64::NZCV, TRI)) {
----------------
Pull `CmpInstr->getParent()` out into its own variable?


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:1665
+  if (NzcvUse)
+    NzcvUse = &NZCVUsedAfterCmp;
+  return (!NZCVUsedAfterCmp.C && !NZCVUsedAfterCmp.V);
----------------
I don't think this actually does anything? It's writing to a local variable.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:1739
 
+/// Check if CmpInstr can be removed and condition code used after must be
+/// inverted.
----------------
I think it'd be good to break this sentence down a little.

```
\returns True if \p CmpInstr can be removed.

\p IsInvertCC is true if, after removing \p CmpInstr, uses of the condition code must be inverted.
```


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:1742
+///
+/// CmpInstr can be removed if:
+/// - CmpInstr is 'ADDS %vreg, 0' or 'SUBS %vreg, 0',
----------------
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.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:1760
+///   'CSINCXr %vreg, xzr, xzr, eq/pl', CmpInstr is 'SUBS %vreg, 1'
+static bool canCmpInstrBeRemoved(MachineInstr *MI, MachineInstr *CmpInstr,
+                                 int CmpValue, const TargetRegisterInfo *TRI,
----------------
These should never be nullptr according to the asserts, so they might as well be passed by reference.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:1762
+                                 int CmpValue, const TargetRegisterInfo *TRI,
+                                 SmallVectorImpl<MachineInstr *> &CCUseInstrs,
+                                 bool &IsInvertCC) {
----------------
Use `ArrayRef`?


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:1790
+  const unsigned CmpOpcode = CmpInstr->getOpcode();
+  if (CmpValue && !isSUBSRegImm(CmpOpcode))
+    return false;
----------------
Pull `isSUBSRegImm` out into a variable and use it in both of the `if` statements?


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:1844
+    MachineInstr &CmpInstr, unsigned SrcReg, int CmpValue,
+    const MachineRegisterInfo *MRI) const {
+  assert(MRI);
----------------
Since `MRI` should never be nullptr, I think it makes sense for it to be a reference.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:1861
+      int Idx = findCondCodeUseOperandIdx(*CCUseInstr);
+      assert(Idx >= 0);
+      MachineOperand &CCOperand = CCUseInstr->getOperand(Idx);
----------------
This assert could use a string explaining it.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:1864
+      AArch64CC::CondCode CCUse = AArch64CC::getInvertedCondCode(
+          (AArch64CC::CondCode)CCOperand.getImm());
+      CCOperand.setImm(CCUse);
----------------
Nit: `static_cast` is easier to grep for


================
Comment at: llvm/test/CodeGen/AArch64/csinc-cmp-removal.mir:1
+# RUN: llc -mtriple=aarch64 -run-pass=peephole-opt -verify-machineinstrs %s -o - | FileCheck %s
+---
----------------
Nit: You can use update_mir_test_checks.py to generate the check lines. This makes it easier to update the test if necessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98564