[AArch64] Type conversion bug when anlyze compare

[David.Xu]

Summary

In analyzeCompare() function in AArch64InstrInfo.cpp, the return val type of decodeLogicalImmediate is uint64_t, while CmpValue is int, it causes a bug when converting uint64_t to int. The bug was found in spec2006-483.xalancbmk. CmpValue is only used to compare with zero in OptimizeCompareInstr(), so we can fix it by comparing it with zero in analyzeCompare().

[t.p.northover]

Shouldn't you be able to produce a test for this? It doesn't seem impossibly difficult to distil the key features from the SPEC case.

I think the change also needs better documentation. You're changing the meaning of CmpValue here: no-one can ever rely on any nonzero value after this commit. That might be OK, since we don't want to anyway right now, but I think it needs to be clear and blow up horribly.

Perhaps making sure CmpValue is always 0 or 1, and asserting that in optimizeCompare? (With an appropriate comments about the limitation and why it was introduced).

Cheers.

Tim.

[David.Xu]

Test case is added in the new patch. Without the patch, an ANDS instruction will be deleted when doing peephole optimization.

[t.p.northover]

Thanks for the test, but what about the other comments? I still think the code change leaves that area too ambiguous.

Cheers.

Tim.

[David.Xu]

Yes, This patch changes the meaning of CmpValue. It may not be the best way to solve this problem, but it has no effect on the other compare instructions and other backends comparing to the way of changing the interface of the function. And appropriate comments are added in optimizeCompare.

[David.Xu]

I have converted all of the CmpValue to 0 or 1, and also added assertion in optimizeCompare. For that our daily performance test have broken down for a long time, please review it ASAP. Thank you!

[t.p.northover]

Thanks David,

I think that looks OK now. Hopefully any future meddlers will notice the warnings.

Cheers.

Tim.

[Eugene.Zelenko]

Committed in rL215206.

lib/Target/AArch64/AArch64InstrInfo.cpp

     SrcReg = MI->getOperand(1).getReg();
     SrcReg2 = 0;
     CmpMask = ~0;
-    CmpValue = MI->getOperand(2).getImm();
+    // FIXME: In order to convert CmpValue to 0 or 1
+    CmpValue = (MI->getOperand(2).getImm() != 0);
     return true;
   case AArch64::ANDSWri:
   case AArch64::ANDSXri:

     SrcReg = MI->getOperand(1).getReg();
     SrcReg2 = 0;
     CmpMask = ~0;
-    CmpValue = AArch64_AM::decodeLogicalImmediate(
-        MI->getOperand(2).getImm(),
-        MI->getOpcode() == AArch64::ANDSWri ? 32 : 64);
+    // FIXME:The return val type of decodeLogicalImmediate is uint64_t,
+    // while the type of CmpValue is int. When converting uint64_t to int,
+    // the high 32 bits of uint64_t will be lost.
+    // In fact it causes a bug in spec2006-483.xalancbmk
+    // CmpValue is only used to compare with zero in OptimizeCompareInstr
+    CmpValue = (AArch64_AM::decodeLogicalImmediate(
+                    MI->getOperand(2).getImm(),
+                    MI->getOpcode() == AArch64::ANDSWri ? 32 : 64) != 0);
     return true;
   }
 

   }
 
   // Continue only if we have a "ri" where immediate is zero.
+  // FIXME:CmpValue has already been converted to 0 or 1 in analyzeCompare
+  // function.
+  assert((CmpValue == 0 || CmpValue == 1) && "CmpValue must be 0 or 1!");
   if (CmpValue != 0 || SrcReg2 != 0)
     return false;
 

test/CodeGen/AArch64/analyzecmp.ll

+; RUN: llc -O3 -mcpu=cortex-a57 < %s | FileCheck %s 
+
+; CHECK-LABLE: @test
+; CHECK: tst [[CMP:x[0-9]+]], #0x8000000000000000
+; CHECK: csel [[R0:x[0-9]+]], [[S0:x[0-9]+]], [[S1:x[0-9]+]], eq
+; CHECK: csel [[R1:x[0-9]+]], [[S2:x[0-9]+]], [[S3:x[0-9]+]], eq
+target datalayout = "e-m:e-i64:64-i128:128-n32:64-S128"
+target triple = "arm64--linux-gnueabi"
+
+define void @test(i64 %a, i64* %ptr1, i64* %ptr2) #0 align 2 {
+entry:
+  %conv = and i64 %a, 4294967295
+  %add = add nsw i64 %conv, -1
+  %div = sdiv i64 %add, 64
+  %rem = srem i64 %add, 64
+  %cmp = icmp slt i64 %rem, 0
+  br i1 %cmp, label %if.then, label %exit
+
+if.then:                                
+  %add2 = add nsw i64 %rem, 64
+  %add3 = add i64 %div, -1
+  br label %exit
+
+exit:                 
+  %__n = phi i64 [ %add3, %if.then ], [ %div, %entry ]
+  %__n.0 = phi i64 [ %add2, %if.then ], [ %rem, %entry ]
+  store i64 %__n, i64* %ptr1
+  store i64 %__n.0, i64* %ptr2
+  ret void 
+}
+
+