[PATCH] D18838: [AArch64][CodeGen] Fix of incorrect peephole optimization in AArch64InstrInfo::optimizeCompareInstr

[Tim Northover via llvm-commits]

t.p.northover added inline comments.

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:993
@@ +992,3 @@
+      NZCV.Z = true;
+    case AArch64CC::HS:
+    case AArch64CC::LO:
----------------
Can you explicitly annotate this fall-through?

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:1005-1006
@@ +1004,4 @@
+    case AArch64CC::VC:
+      NZCV.V = true;
+
+    case AArch64CC::GT:
----------------
This shouldn't be a fallthrough if I'm reading the manual correctly (definition of `ConditionHolds` on page J1-5267 for example).

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:1044-1045
@@ +1043,4 @@
+  const unsigned CmpOpcode = CmpInstr->getOpcode();
+  if (!isADDSRegImm(CmpOpcode) && !isSUBSRegImm(CmpOpcode))
+    return false;
+
----------------
Do we ever check that they're comparing agains the same value as MI? (Always 0 I believe).

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:1053-1055
@@ +1052,5 @@
+
+  UsedNZCV NZCVUsedBetweenMIAndCmp;
+  for (auto I = std::next(MI->getIterator()), E = CmpInstr->getIterator();
+       I != E; ++I) {
+    const MachineInstr &Instr = *I;
----------------
What's allowed between MI and CmpInstr depends on what MI is:

  * If it's an ADDS/SUBS already then any use of flags is permitted and only definitions are wrong.
  * If it's an ADD/SUB then neither uses nor defs are allowed (of any flags). Uses imply NZCV is already live and we're going to clobber it; defs imply it would be clobbered before it reached CmpInstr.

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:1083-1084
@@ +1082,4 @@
+  
+  return !NZCVUsedBetweenMIAndCmp.C && !NZCVUsedBetweenMIAndCmp.V
+         && !NZCVUsedAfterCmp.C && !NZCVUsedAfterCmp.V;
+}
----------------
I think this function should either return an NZCVUsed (to be checked by the caller) or check the forms of MI and CmpInstr itself. If they're SUBS/CMP or ADDS/CMN then the substitution is valid regardless of flags used.

================
Comment at: test/CodeGen/AArch64/arm64-regress-opt-cmp.mir:1
@@ +1,2 @@
+# RUN: llc -mtriple=aarch64-linux-gnu -run-pass peephole-opts %s 2>&1 | FileCheck %s
+# CHECK: %8 = LSLVWr {{.*}}
----------------
This test has lots of extra cruft and only ends up checking one instance anyway. The point of MIR tests is that we can exercise more aspects of the logic than from IR alone.


http://reviews.llvm.org/D18838