[PATCH] [SeparateConstOffsetFromGEP] Bail there is only one GEP offset

[James Molloy]

Hi Jingue,

What could %offset be re associated with? It's just one variable- ok it
could be multiplied by something, but the only real operation you can do on
it is strength reduction.

Enabling it pessimizes a bunch of code by creating unnecessary ptrtoints
that the optimiser isn't as good at handling as GEPs. As a matter of fact,
this patch is reenabling some aarch64 tests that currently fail.

Can you give an example of when this would be useful?
On Tue, 21 Apr 2015 at 17:45, David Blaikie <dblaikie at gmail.com> wrote:

> On Tue, Apr 21, 2015 at 6:54 AM, James Molloy <james.molloy at arm.com>
> wrote:
>
>> Hi jingyue, wu, Gerolf,
>>
>> A single-offset GEP such as:
>>
>>   getelementptr i8*, i8* %base, i32 %offset
>>
>
> Just out of pedantry/by way of explanation, that should be: getelementptr
> i8, i8* %base, ....
>
> (note the first type parameter is the type pointed to, not the pointer
> type - this is part of the transition to opaque pointer types - so there
> will be no "pointer to i8" in the future, just a raw pointer and operations
> like gep will indicate how to treat the referenced bytes)
>
>
>>
>> doesn't make sense to split apart. There is no complex calculation to CSE.
>>
>> REPOSITORY
>>   rL LLVM
>>
>> http://reviews.llvm.org/D9149
>>
>> Files:
>>   lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp
>>   test/CodeGen/AArch64/arm64-addr-mode-folding.ll
>>   test/CodeGen/AArch64/arm64-cse.ll
>>   test/Transforms/SeparateConstOffsetFromGEP/NVPTX/split-gep.ll
>>
>> Index: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp
>> ===================================================================
>> --- lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp
>> +++ lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp
>> @@ -829,7 +829,7 @@
>>
>>    // The backend can already nicely handle the case where all indices are
>>    // constant.
>> -  if (GEP->hasAllConstantIndices())
>> +  if (GEP->hasAllConstantIndices() || GEP->getNumOperands() == 2)
>>      return false;
>>
>>    bool Changed = canonicalizeArrayIndicesToPointerSize(GEP);
>> Index: test/CodeGen/AArch64/arm64-addr-mode-folding.ll
>> ===================================================================
>> --- test/CodeGen/AArch64/arm64-addr-mode-folding.ll
>> +++ test/CodeGen/AArch64/arm64-addr-mode-folding.ll
>> @@ -1,4 +1,4 @@
>> -; RUN: llc -O3 -mtriple arm64-apple-ios3 -aarch64-gep-opt=false %s -o -
>> | FileCheck %s
>> +; RUN: llc -O3 -mtriple arm64-apple-ios3 %s -o - | FileCheck %s
>>  ; <rdar://problem/13621857>
>>
>>  @block = common global i8* null, align 8
>> Index: test/CodeGen/AArch64/arm64-cse.ll
>> ===================================================================
>> --- test/CodeGen/AArch64/arm64-cse.ll
>> +++ test/CodeGen/AArch64/arm64-cse.ll
>> @@ -1,4 +1,4 @@
>> -; RUN: llc -O3 < %s -aarch64-atomic-cfg-tidy=0 -aarch64-gep-opt=false
>> -verify-machineinstrs | FileCheck %s
>> +; RUN: llc -O3 < %s -aarch64-atomic-cfg-tidy=0 -verify-machineinstrs |
>> FileCheck %s
>>  target triple = "arm64-apple-ios"
>>
>>  ; rdar://12462006
>> @@ -12,7 +12,7 @@
>>  ; CHECK-NOT: sub
>>  ; CHECK: b.ge
>>  ; CHECK: sub
>> -; CHECK: sub
>> +; CHECK: {{sub|add}}
>>  ; CHECK-NOT: sub
>>  ; CHECK: ret
>>   %0 = load i32, i32* %offset, align 4
>> Index: test/Transforms/SeparateConstOffsetFromGEP/NVPTX/split-gep.ll
>> ===================================================================
>> --- test/Transforms/SeparateConstOffsetFromGEP/NVPTX/split-gep.ll
>> +++ test/Transforms/SeparateConstOffsetFromGEP/NVPTX/split-gep.ll
>> @@ -237,7 +237,7 @@
>>
>>  ; The code that rebuilds an OR expression used to be buggy, and failed
>> on this
>>  ; test.
>> -define float* @shl_add_or(i64 %a, float* %ptr) {
>> +define float* @shl_add_or(i64 %a, [10 x float]* %ptr) {
>>  ; CHECK-LABEL: @shl_add_or(
>>  entry:
>>    %shl = shl i64 %a, 2
>> @@ -247,8 +247,8 @@
>>    ; ((a << 2) + 12) and 1 have no common bits. Therefore,
>>    ; SeparateConstOffsetFromGEP is able to extract the 12.
>>    ; TODO(jingyue): We could reassociate the expression to combine 12 and
>> 1.
>> -  %p = getelementptr float, float* %ptr, i64 %or
>> -; CHECK: [[PTR:%[a-zA-Z0-9]+]] = getelementptr float, float* %ptr, i64
>> [[OR]]
>> +  %p = getelementptr [10 x float], [10 x float]* %ptr, i64 %or, i64 0
>> +; CHECK: [[PTR:%[a-zA-Z0-9]+]] = getelementptr [10 x float], [10 x
>> float]* %ptr, i64 [[OR]], i64 0
>>  ; CHECK: getelementptr float, float* [[PTR]], i64 12
>>    ret float* %p
>>  ; CHECK-NEXT: ret
>>
>> EMAIL PREFERENCES
>>   http://reviews.llvm.org/settings/panel/emailpreferences/
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150421/c7c9a0ce/attachment.html>