[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:1044-1045
@@ +1043,4 @@
+  const unsigned CmpOpcode = CmpInstr->getOpcode();
+  if (!isADDSRegImm(CmpOpcode) && !isSUBSRegImm(CmpOpcode))
+    return false;
+
----------------
eastig wrote:
> 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?
Sorry about that, I was talking nonsense. I forgot that SrcReg was the **destination** of the candidate instruction so comparison against 0 was automatic if flags were set. No need to support anything else.

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:1083-1084
@@ +1082,4 @@
+  
+  return !NZCVUsedBetweenMIAndCmp.C && !NZCVUsedBetweenMIAndCmp.V
+         && !NZCVUsedAfterCmp.C && !NZCVUsedAfterCmp.V;
+}
----------------
eastig wrote:
> 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
Oh, of course. Same flawed reasoning from me as above I think.


http://reviews.llvm.org/D18838