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

[Evgeny Astigeevich via llvm-commits]

eastig added a comment.

Hi Tim,

Thank you for comments. See my answers.
I am updating the patch. Need some time. Quite busy on armcc.


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:993
@@ +992,3 @@
+      NZCV.Z = true;
+    case AArch64CC::HS:
+    case AArch64CC::LO:
----------------
t.p.northover wrote:
> Can you explicitly annotate this fall-through?
I will add comments to each case to show which flags are used.

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:1005-1006
@@ +1004,4 @@
+    case AArch64CC::VC:
+      NZCV.V = true;
+
+    case AArch64CC::GT:
----------------
t.p.northover wrote:
> This shouldn't be a fallthrough if I'm reading the manual correctly (definition of `ConditionHolds` on page J1-5267 for example).
Good catch. Thank you.
It's a bug. 'break' is missed. 

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:1044-1045
@@ +1043,4 @@
+  const unsigned CmpOpcode = CmpInstr->getOpcode();
+  if (!isADDSRegImm(CmpOpcode) && !isSUBSRegImm(CmpOpcode))
+    return false;
+
----------------
t.p.northover wrote:
> Do we ever check that they're comparing agains the same value as MI? (Always 0 I believe).
I will rename 'substituteCmpInstr' to 'substituteCmpToZero' to be clear what is the case. I don't think we need to check that MI and CmpInstr use the same value. 
I added these checks because 'optimizeCompareInstr' is called for instructions which are supported by 'analyzeCompare'. Currently they are ADDS, SUBS and ANDS. We cannot substitute ANDS because 'ANDS vreg, 0'  always produces 0. We can substitute 'ANDS vreg, -1' but it's not comparision with zero. Is this case worth?

================
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;
----------------
t.p.northover wrote:
> 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.
You are right.
I would rewrite this as follows:
  - If MI opcode is the S form there must be no defs of flags.
  - If MI opcode is not the S form there must be neither defs of flags nor uses of flags.


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:1083-1084
@@ +1082,4 @@
+  
+  return !NZCVUsedBetweenMIAndCmp.C && !NZCVUsedBetweenMIAndCmp.V
+         && !NZCVUsedAfterCmp.C && !NZCVUsedAfterCmp.V;
+}
----------------
t.p.northover wrote:
> 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.
We can only be sure for N and Z flags. SUBS/CMP or ADDS/CMN can produce different C and V flags, e.g.

  %vr2  =  SUBS %vr1,  1 ; sets C to 0 when %vr1 == 0
  %cmp = SUBS %vr2, 0  ; sets C to 1

================
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 {{.*}}
----------------
t.p.northover wrote:
> 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.
Yes, it looks cumbersome. I am reducing it as much as possible.


http://reviews.llvm.org/D18838