[llvm] r262248 - [AArch64] Fix isLegalAddImmediate() to return true for valid negative values.

[Geoff Berry via llvm-commits]

Most likely what is going on is that making isLegalAddImmediate() more precise is leading LoopStrengthReduce to make a different decision about some IV solution, which happens to be worse in this case.

I saw a bit of this in SPEC, but the net perf delta was positive, and the change itself seems good in isolation.

 

I’ll investigate this case to see if my suspicion is correct, and if so if there’s anything we can do about it.  

 

--

Geoff Berry

Employee of Qualcomm Innovation Center, Inc.

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

 

From: James Molloy [mailto:james at jamesmolloy.co.uk] 
Sent: Tuesday, March 01, 2016 11:31 AM
To: Geoff Berry; llvm-commits at lists.llvm.org
Subject: Re: [llvm] r262248 - [AArch64] Fix isLegalAddImmediate() to return true for valid negative values.

 

Hi Geoff,

 

This commit caused a 10% regression in huffbench: http://llvm.org/perf/db_default/v4/nts/daily_report/2016/3/1?filter-machine-regex=aarch64%7Carm%7Cgreen

 

There are also many other tests in that report that have regressed that I suspect to be the same revision. We've also seen quite large swings (not all negative!) on internal/third party tests.

 

Could you please take a look at what could be going on?

 

Cheers,

 

James

 

On Mon, 29 Feb 2016 at 19:57 Geoff Berry via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org> > wrote:

Author: gberry
Date: Mon Feb 29 13:53:22 2016
New Revision: 262248

URL: http://llvm.org/viewvc/llvm-project?rev=262248 <http://llvm.org/viewvc/llvm-project?rev=262248&view=rev> &view=rev
Log:
[AArch64] Fix isLegalAddImmediate() to return true for valid negative values.

Reviewers: t.p.northover, jmolloy

Subscribers: mcrosier, aemerson, llvm-commits, rengolin

Differential Revision: http://reviews.llvm.org/D17463

Added:
    llvm/trunk/test/CodeGen/AArch64/neg-imm.ll
Modified:
    llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.cpp

Modified: llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.cpp?rev=262248 <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.cpp?rev=262248&r1=262247&r2=262248&view=diff> &r1=262247&r2=262248&view=diff
==============================================================================
--- llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.cpp (original)
+++ llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.cpp Mon Feb 29 13:53:22 2016
@@ -7280,6 +7280,8 @@ EVT AArch64TargetLowering::getOptimalMem

 // 12-bit optionally shifted immediates are legal for adds.
 bool AArch64TargetLowering::isLegalAddImmediate(int64_t Immed) const {
+  // Same encoding for add/sub, just flip the sign.
+  Immed = std::abs(Immed);
   if ((Immed >> 12) == 0 || ((Immed & 0xfff) == 0 && Immed >> 24 == 0))
     return true;
   return false;
@@ -7288,8 +7290,6 @@ bool AArch64TargetLowering::isLegalAddIm
 // Integer comparisons are implemented with ADDS/SUBS, so the range of valid
 // immediates is the same as for an add or a sub.
 bool AArch64TargetLowering::isLegalICmpImmediate(int64_t Immed) const {
-  if (Immed < 0)
-    Immed *= -1;
   return isLegalAddImmediate(Immed);
 }


Added: llvm/trunk/test/CodeGen/AArch64/neg-imm.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/AArch64/neg-imm.ll?rev=262248 <http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/AArch64/neg-imm.ll?rev=262248&view=auto> &view=auto
==============================================================================
--- llvm/trunk/test/CodeGen/AArch64/neg-imm.ll (added)
+++ llvm/trunk/test/CodeGen/AArch64/neg-imm.ll Mon Feb 29 13:53:22 2016
@@ -0,0 +1,46 @@
+; RUN: llc -mtriple=aarch64-linux-gnu -verify-machineinstrs -o - %s | FileCheck %s
+; LSR used to pick a sub-optimal solution due to the target responding
+; conservatively to isLegalAddImmediate for negative values.
+
+declare void @foo(i32)
+
+define void @test(i32 %px) {
+; CHECK-LABEL: test:
+; CHECK: // %entry
+; CHECK: subs
+; CHECK-NEXT: csel
+entry:
+  %sub = add nsw i32 %px, -1
+  %cmp = icmp slt i32 %px, 1
+  %.sub = select i1 %cmp, i32 0, i32 %sub
+  br label %for.body
+
+for.body:
+; CHECK: // %for.body
+; CHECK:  cmp
+; CHECK-NEXT:  b.eq
+; CHECK: // %if.then3
+  %x.015 = phi i32 [ %inc, %for.inc ], [ %.sub, %entry ]
+  %cmp2 = icmp eq i32 %x.015, %px
+  br i1 %cmp2, label %for.inc, label %if.then3
+
+if.then3:
+  tail call void @foo(i32 %x.015)
+  br label %for.inc
+
+for.inc:
+; CHECK: // %for.inc
+; CHECK:  add
+; CHECK-NEXT:  cmp
+; CHECK:  b.le
+; CHECK: // %for.cond.cleanup
+  %inc = add nsw i32 %x.015, 1
+  %cmp1 = icmp sgt i32 %x.015, %px
+  br i1 %cmp1, label %for.cond.cleanup.loopexit, label %for.body
+
+for.cond.cleanup.loopexit:
+  br label %for.cond.cleanup
+
+for.cond.cleanup:
+  ret void
+}


_______________________________________________
llvm-commits mailing list
llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org> 
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160301/cbc34763/attachment.html>