[GVN] Don't perform scalar PRE on GEPs

[labrinea]

Summary

Partial Redundancy Elimination of GEPs prevents CodeGenPrepare from sinking the addressing mode computation of memory instructions back to its uses. The problem comes from the insertion of PHIs, which confuse CGP and make it bail.

I found this problem when looking at sqlite amalgamation from https://www.sqlite.org/download.html.

We could teach CGP to look through PHI nodes in FindAllMemoryUses but this would increase the compilation time (currently scanning is limited to 20 memory instructions - sqlite needs 6 times more). Moreover, CGP still wouldn't be able to handle GEPs that have different base and offset but correspond to the same Value Number (like in the regression test).

This looks good for performance and codesize. I am posting some performance numbers targeting Cortex-A57 AArch64 reported by LNT for llvm-test-suite, spec2000, and spec2006 at -O3 using a resent LLVM trunk revision with my patch applied.

Performance Improvements - execution_time
MultiSource/Benchmarks/FreeBench/mason/mason -15.28%
External/SPEC/CINT2000/253.perlbmk/253.perlbmk -4.07%
External/SPEC/CINT2006/483.xalancbmk/483.xalancbmk -3.38%
External/SPEC/CINT2006/401.bzip2/401.bzip2 -2.82%
MultiSource/Benchmarks/Olden/em3d/em3d -2.81%
SingleSource/Benchmarks/Shootout-C++/Shootout-C++-heapsort -2.67%
SingleSource/Benchmarks/Shootout/Shootout-heapsort -2.24%
MultiSource/Benchmarks/Bullet/bullet -1.37%
SingleSource/Benchmarks/Adobe-C++/stepanov_vector -1.15%

Performance Regressions - execution_time
External/SPEC/CINT2006/400.perlbench/400.perlbench 1.45%

Performance Improvements - mem_bytes
MultiSource/Benchmarks/MiBench/automotive-susan/automotive-susan -2.68%
MultiSource/Benchmarks/Olden/tsp/tsp -2.14%
MultiSource/Benchmarks/FreeBench/mason/mason -1.27%

[john.brawn]

It may be worthwhile allowing scalar PRE on GEPs that we know won't be combined into the addressing mode of a load/store, i.e. those where TargetTransformInfo::isLegalAddressingMode returns false.

[labrinea]

In D55009#1311601, @john.brawn wrote:

It may be worthwhile allowing scalar PRE on GEPs that we know won't be combined into the addressing mode of a load/store, i.e. those where TargetTransformInfo::isLegalAddressingMode returns false.

That would require to run the Target IR Analysis before GVN. It would also require an Address Mode Matcher like the one CodeGenPrepare implements. It doesn't seem worthwhile to me unless there's another way I haven't thought about.

[john.brawn]

The idea behind allowing Scalar PRE when the addressing mode isn't legal is for some thing like (adjusted from pre-gep-load.ll)

define void @foo(i32 %stat, i32 %i, double* %p, double* %q) {
entry:
  switch i32 %stat, label %sw.default [
    i32 0, label %sw.bb
    i32 1, label %sw.bb
    i32 2, label %sw.bb2
  ]

sw.bb:                                            ; preds = %entry, %entry
  %arrayidx1 = getelementptr inbounds double, double* %p, i64 1234567
  store double 1.0, double* %arrayidx1, align 8
  br i1 undef, label %if.then, label %if.end

if.then:                                          ; preds = %sw.bb
  br label %return

if.end:                                           ; preds = %sw.bb
  br label %sw.bb2

sw.bb2:                                           ; preds = %if.end, %entry
  %arrayidx5 = getelementptr inbounds double, double* %p, i64 1234567
  store double 0.0, double* %arrayidx5, align 8
  br label %return

sw.default:                                       ; preds = %entry
  br label %return

return:                                           ; preds = %sw.default, %sw.bb2, %if.then
  ret void
}

The offset in the gep is too big to use as an immediate offset in AArch64, so we'd like to not materialise the same constant twice to use as the offset, but doing PRE on the GEP means we get

	mov	w8, #46136
	movk	w8, #150, lsl #16
	add	x8, x2, x8
.LBB0_4:                                // %sw.bb2
	str	xzr, [x8]

but we really want

	mov	w8, #46136
	movk	w8, #150, lsl #16
.LBB0_4:                                // %sw.bb2
	str	xzr, [x2, x8]

i.e. we'd like to PRE the offset generation but not the add. So I think that side of things is better handled by some kind of MachinePRE, and for GVN it's fine to just refuse to do scalar PRE on GEPs (i.e. what this patch does).

[john.brawn]

test/Transforms/GVN/PRE/pre-gep-load.ll

1

The version of this file that I see in trunk doesn't have these autogenerated check lines. Is there intended to be a commit before this that adjusts the test?

[labrinea]

test/Transforms/GVN/PRE/pre-gep-load.ll

1

Indeed. I've just added them in this revision to demonstrate the difference. I wasn't intending to do a separate commit unless it's necessary.

[john.brawn]

OK, then this looks good to me.

lib/Transforms/Scalar/GVN.cpp

bool GVN::performScalarPRE(Instruction *CurInst) {
 
   // Don't do PRE on compares. The PHI would prevent CodeGenPrepare from
   // sinking the compare again, and it would force the code generator to
   // move the i1 from processor flags or predicate registers into a general
   // purpose register.
   if (isa<CmpInst>(CurInst))
     return false;
 
+  // Don't do PRE on GEPs. The inserted PHI would prevent CodeGenPrepare from
+  // sinking the addressing mode computation back to its uses. Extending the
+  // GEP's live range increases the register pressure, and therefore it can
+  // introduce unnecessary spills.
+  //
+  // This doesn't prevent Load PRE. PHI translation will make the GEP available
+  // to the load by moving it to the predecessor block if necessary.
+  if (isa<GetElementPtrInst>(CurInst))
+    return false;
+
   // We don't currently value number ANY inline asm calls.
   if (CallInst *CallI = dyn_cast<CallInst>(CurInst))
     if (CallI->isInlineAsm())
       return false;
 
   uint32_t ValNo = VN.lookup(CurInst);
 
   // Look for the predecessors for PRE opportunities.  We're

test/Transforms/GVN/PRE/pre-gep-load.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt < %s -basicaa -gvn -enable-load-pre -S | FileCheck %s
 ; RUN: opt < %s -aa-pipeline=basic-aa -passes=gvn -enable-load-pre -S | FileCheck %s
 
 target datalayout = "e-m:e-i64:64-i128:128-n32:64-S128"
 target triple = "aarch64--linux-gnu"
 
 define double @foo(i32 %stat, i32 %i, double** %p) {
 ; CHECK-LABEL: @foo(

 ; CHECK-NEXT:    [[SUB:%.*]] = fsub double [[TMP1]], 1.000000e+00
 ; CHECK-NEXT:    [[CMP:%.*]] = fcmp olt double [[SUB]], 0.000000e+00
 ; CHECK-NEXT:    br i1 [[CMP]], label [[IF_THEN:%.*]], label [[IF_END:%.*]]
 ; CHECK:       if.then:
 ; CHECK-NEXT:    br label [[RETURN:%.*]]
 ; CHECK:       if.end:
 ; CHECK-NEXT:    br label [[SW_BB2]]
 ; CHECK:       sw.bb2:
-; CHECK-NEXT:    [[ARRAYIDX5_PRE_PHI:%.*]] = phi double* [ [[ARRAYIDX5_PHI_TRANS_INSERT]], [[ENTRY_SW_BB2_CRIT_EDGE]] ], [ [[ARRAYIDX1]], [[IF_END]] ]
 ; CHECK-NEXT:    [[TMP2:%.*]] = phi double [ [[DOTPRE2]], [[ENTRY_SW_BB2_CRIT_EDGE]] ], [ [[TMP1]], [[IF_END]] ]
 ; CHECK-NEXT:    [[IDXPROM3_PRE_PHI:%.*]] = phi i64 [ [[DOTPRE1]], [[ENTRY_SW_BB2_CRIT_EDGE]] ], [ [[IDXPROM]], [[IF_END]] ]
 ; CHECK-NEXT:    [[TMP3:%.*]] = phi double* [ [[DOTPRE]], [[ENTRY_SW_BB2_CRIT_EDGE]] ], [ [[TMP0]], [[IF_END]] ]
+; CHECK-NEXT:    [[ARRAYIDX5:%.*]] = getelementptr inbounds double, double* [[TMP3]], i64 [[IDXPROM3_PRE_PHI]]
 ; CHECK-NEXT:    [[SUB6:%.*]] = fsub double 3.000000e+00, [[TMP2]]
-; CHECK-NEXT:    store double [[SUB6]], double* [[ARRAYIDX5_PRE_PHI]]
+; CHECK-NEXT:    store double [[SUB6]], double* [[ARRAYIDX5]]
 ; CHECK-NEXT:    br label [[RETURN]]
 ; CHECK:       sw.default:
 ; CHECK-NEXT:    br label [[RETURN]]
 ; CHECK:       return:
 ; CHECK-NEXT:    [[RETVAL_0:%.*]] = phi double [ 0.000000e+00, [[SW_DEFAULT]] ], [ [[SUB6]], [[SW_BB2]] ], [ [[SUB]], [[IF_THEN]] ]
 ; CHECK-NEXT:    ret double [[RETVAL_0]]
 ;
 entry: