[SeparateConstOffsetFromGEP] Bail there is only one GEP offset

[jingyue]

Summary

A single-offset GEP such as:

getelementptr i8*, i8* %base, i32 %offset

doesn't make sense to split apart. There is no complex calculation to CSE.

[jingyue]

Hi James,

I don't get why

getelementptr i8*, i8* %base, i32 %offset

is not worth splitting if %offset can be reassociated to variadic+const. base+variadic may still be CSE'ed in that case.

[jingyue]

For example,

p1 = &base[offset + 1];
p2 = &base[offset + 2];

Reassociating GEPs gives us

p1 = (&base[offset]) + 1
p2 = (&base[offset]) + 2

where &base[offset] can be CSE'ed.

Can you show me the code patterns you discovered that demonstrates the
regression? I can think about how to address them too. Thanks!

Jingyue

[Gerolf]

lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp

832

Please comment on the rationale for ==2. Is there a more general cost model problem?

[jingyue]

Oops... I pressed the wrong button. I meant to say "need revision". jmolly@, feel free to reclaim it.

lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp

 
 bool SeparateConstOffsetFromGEP::splitGEP(GetElementPtrInst *GEP) {
   // Skip vector GEPs.
   if (GEP->getType()->isVectorTy())
     return false;
 
   // 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);
 
   bool NeedsExtraction;
   int64_t AccumulativeByteOffset = accumulateByteOffset(GEP, NeedsExtraction);
 
   if (!NeedsExtraction)

test/CodeGen/AArch64/arm64-addr-mode-folding.ll

-; 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
 
 define i32 @fct(i32 %i1, i32 %i2) {
 ; CHECK: @fct
 ; Sign extension is used more than once, thus it should not be folded.
 ; CodeGenPrepare is not sharing sext across uses, thus this is folded because

test/CodeGen/AArch64/arm64-cse.ll

-; 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
 ; CSE between "icmp reg reg" and "sub reg reg".
 ; Both can be in the same basic block or in different basic blocks.
 define i8* @t1(i8* %base, i32* nocapture %offset, i32 %size) nounwind {
 entry:
 ; CHECK-LABEL: t1:
 ; CHECK: subs
 ; CHECK-NOT: cmp
 ; 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
  %cmp = icmp slt i32 %0, %size
  %s = sub nsw i32 %0, %size
  br i1 %cmp, label %return, label %if.end
 
 if.end:

test/Transforms/SeparateConstOffsetFromGEP/NVPTX/split-gep.ll

entry:
   ret float* %p
 }
 ; CHECK-LABEL: @and(
 ; CHECK: getelementptr [32 x [32 x float]], [32 x [32 x float]]* @float_2d_array
 ; CHECK-NOT: getelementptr
 
 ; 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
   %add = add i64 %shl, 12
   %or = or i64 %add, 1
 ; CHECK: [[OR:%or[0-9]*]] = add i64 %shl, 1
   ; ((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
 }
 
 ; The source code used to be buggy in checking
 ; (AccumulativeByteOffset % ElementTypeSizeOfGEP == 0)
 ; where AccumulativeByteOffset is signed but ElementTypeSizeOfGEP is unsigned.