[llvm-commits] [llvm] r158237 - in /llvm/trunk: lib/Transforms/InstCombine/InstCombineAddSub.cpp test/Transforms/InstCombine/pr12338.ll

[Nuno Lopes]

It's true that in general instcombine tries to push immediates to the RHS. 
But the case of '%b = sub i32 42, %a' is an exception.
The deal is that to represent such a subtraction but with the immediate on 
RHS, one needs an additional negate instruction (which is nothing more than 
'sub 0, %foo'). So you would end up with an immediate in LHS anyway.

So the choices are:
%b = sub i32 42, %a
vs
%tmp = add i32 %a, -42
%b = sub i32 0, %tmp

Instcombine prefers the first form (and already did before my change).

Nuno

P.S.: Forgot to acknowledge in the commit, but thanks to Nick Lewycky for 
reducing the test case!


-----Original Message-----
From: Evan Cheng
Sent: Tuesday, June 12, 2012 2:37 AM

Is this really right? I thought LLVM passes like to canonicalize expressions 
so the immediate is always the RHS.

Evan

On Jun 8, 2012, at 3:30 PM, Nuno Lopes wrote:

> Author: nlopes
> Date: Fri Jun  8 17:30:05 2012
> New Revision: 158237
>
> URL: http://llvm.org/viewvc/llvm-project?rev=158237&view=rev
> Log:
> canonicalize:
> -%a + 42
> into
> 42 - %a
>
> previously we were emitting:
> -(%a + 42)
>
> This fixes the infinite loop in PR12338. The generated code is still not 
> perfect, though.
> Will work on that next
>
> Added:
>    llvm/trunk/test/Transforms/InstCombine/pr12338.ll
> Modified:
>    llvm/trunk/lib/Transforms/InstCombine/InstCombineAddSub.cpp
>
> Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombineAddSub.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineAddSub.cpp?rev=158237&r1=158236&r2=158237&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Transforms/InstCombine/InstCombineAddSub.cpp (original)
> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineAddSub.cpp Fri Jun  8 
> 17:30:05 2012
> @@ -170,10 +170,11 @@
>   // -A + B  -->  B - A
>   // -A + -B  -->  -(A + B)
>   if (Value *LHSV = dyn_castNegVal(LHS)) {
> -    if (Value *RHSV = dyn_castNegVal(RHS)) {
> -      Value *NewAdd = Builder->CreateAdd(LHSV, RHSV, "sum");
> -      return BinaryOperator::CreateNeg(NewAdd);
> -    }
> +    if (!isa<Constant>(RHS))
> +      if (Value *RHSV = dyn_castNegVal(RHS)) {
> +        Value *NewAdd = Builder->CreateAdd(LHSV, RHSV, "sum");
> +        return BinaryOperator::CreateNeg(NewAdd);
> +      }
>
>     return BinaryOperator::CreateSub(RHS, LHSV);
>   }
>
> Added: llvm/trunk/test/Transforms/InstCombine/pr12338.ll
> URL: 
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/pr12338.ll?rev=158237&view=auto
> ==============================================================================
> --- llvm/trunk/test/Transforms/InstCombine/pr12338.ll (added)
> +++ llvm/trunk/test/Transforms/InstCombine/pr12338.ll Fri Jun  8 17:30:05 
> 2012
> @@ -0,0 +1,24 @@
> +; RUN: opt < %s -instcombine -S | FileCheck %s
> +
> +define void @entry() nounwind {
> +entry:
> +  br label %for.cond
> +
> +for.cond:
> +  %local = phi <1 x i32> [ <i32 0>, %entry ], [ %phi2, %cond.end47 ]
> +; CHECK: sub <1 x i32> <i32 92>, %local
> +  %phi3 = sub <1 x i32> zeroinitializer, %local
> +  br label %cond.end
> +
> +cond.false:
> +  br label %cond.end
> +
> +cond.end:
> +  %cond = phi <1 x i32> [ %phi3, %for.cond ], [ undef, %cond.false ]
> +  br label %cond.end47
> +
> +cond.end47:
> +  %sum = add <1 x i32> %cond, <i32 92>
> +  %phi2 = sub <1 x i32> zeroinitializer, %sum
> +  br label %for.cond
> +}