[llvm-commits] [llvm] r173289 - in /llvm/trunk: lib/Analysis/ConstantFolding.cpp test/Transforms/InstCombine/load3.ll

[Duncan Sands]

Hi Benjamin,

On 23/01/13 21:41, Benjamin Kramer wrote:
> Author: d0k
> Date: Wed Jan 23 14:41:05 2013
> New Revision: 173289
>
> URL: http://llvm.org/viewvc/llvm-project?rev=173289&view=rev
> Log:
> ConstantFolding: Evaluate GEP indices in the index type.

are you sure this is correct?  The LangRef is slightly ambiguous, but it sounds
like it is saying that everything is extended to the size of a pointer before
doing arithmetic:

  " If the inbounds keyword is not present, the offsets are added to the base
    address with silently-wrapping two’s complement arithmetic. If the offsets
    have a different width from the pointer, they are sign-extended or truncated
    to the width of the pointer "

However it does say the "offsets" are extended, not the GEP indices, so this
might mean that first you do the scaling in the index type, and then the scaled
result is extended to the width of the pointer.

If the scaling is done in the type of the index then your patch is correct.
But if the scaling is done after extending to the width of a pointer and not
before then your patch is not correct.  In any case, it would be good to be
more explicit in the LangRef.

Ciao, Duncan.

>
> This fixes some edge cases that we would get wrong with uint64_ts.
> PR14986.
>
> Modified:
>      llvm/trunk/lib/Analysis/ConstantFolding.cpp
>      llvm/trunk/test/Transforms/InstCombine/load3.ll
>
> Modified: llvm/trunk/lib/Analysis/ConstantFolding.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/ConstantFolding.cpp?rev=173289&r1=173288&r2=173289&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Analysis/ConstantFolding.cpp (original)
> +++ llvm/trunk/lib/Analysis/ConstantFolding.cpp Wed Jan 23 14:41:05 2013
> @@ -254,13 +254,22 @@
>         if (!CI) return false;  // Index isn't a simple constant?
>         if (CI->isZero()) continue;  // Not adding anything.
>
> +      // Evaluate offsets in the index type.
> +      APInt APOffset(CI->getBitWidth(), Offset);
> +
>         if (StructType *ST = dyn_cast<StructType>(*GTI)) {
>           // N = N + Offset
> -        Offset += TD.getStructLayout(ST)->getElementOffset(CI->getZExtValue());
> +        APOffset +=
> +          APInt(CI->getBitWidth(),
> +                TD.getStructLayout(ST)->getElementOffset(CI->getZExtValue()));
>         } else {
>           SequentialType *SQT = cast<SequentialType>(*GTI);
> -        Offset += TD.getTypeAllocSize(SQT->getElementType())*CI->getSExtValue();
> +        APOffset +=
> +          APInt(CI->getBitWidth(),
> +                TD.getTypeAllocSize(SQT->getElementType())*CI->getSExtValue());
>         }
> +
> +      Offset = APOffset.getSExtValue();
>       }
>       return true;
>     }
>
> Modified: llvm/trunk/test/Transforms/InstCombine/load3.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/load3.ll?rev=173289&r1=173288&r2=173289&view=diff
> ==============================================================================
> --- llvm/trunk/test/Transforms/InstCombine/load3.ll (original)
> +++ llvm/trunk/test/Transforms/InstCombine/load3.ll Wed Jan 23 14:41:05 2013
> @@ -1,6 +1,6 @@
>   ; RUN: opt < %s -instcombine -S | FileCheck %s
> -target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64"
> -target triple = "x86_64-apple-darwin10.0.0"
> +target datalayout = "e-p:32:32:32-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:32:64-f32:32:32-f64:32:64-v64:64:64-v128:128:128-a0:0:64-f80:128:128-n8:16:32-S128"
> +target triple = "i386-apple-macosx10.0.0"
>
>   ; Instcombine should be able to do trivial CSE of loads.
>
> @@ -24,4 +24,23 @@
>
>   ; CHECK: @test2
>   ; CHECK: ret float 0x3806965600000000
> -}
> \ No newline at end of file
> +}
> +
> + at rslts32 = global [36 x i32] zeroinitializer, align 4
> +
> + at expect32 = internal constant [36 x i32][ i32 1, i32 2, i32 0, i32 100, i32 3,
> +i32 4, i32 0, i32 -7, i32 4, i32 4, i32 8, i32 8, i32 1, i32 3, i32 8, i32 3,
> +i32 4, i32 -2, i32 2, i32 8, i32 83, i32 77, i32 8, i32 17, i32 77, i32 88, i32
> +22, i32 33, i32 44, i32 88, i32 77, i32 4, i32 4, i32 7, i32 -7, i32 -8] ,
> +align 4
> +
> +; PR14986
> +define void @test3() nounwind {
> +; This is a weird way of computing zero.
> +  %l = load i32* getelementptr ([36 x i32]* @expect32, i32 29826161, i32 28), align 4
> +  store i32 %l, i32* getelementptr ([36 x i32]* @rslts32, i32 29826161, i32 28), align 4
> +  ret void
> +
> +; CHECK: @test3
> +; CHECK: store i32 1, i32* getelementptr inbounds ([36 x i32]* @rslts32, i32 0, i32 0)
> +}
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>