[PATCH] D55009: [GVN] Don't perform scalar PRE on GEPs

[Alexandros Lamprineas via Phabricator via llvm-commits]

labrinea created this revision.
labrinea added a reviewer: llvm-commits.
Herald added subscribers: kristof.beyls, javed.absar.

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%


https://reviews.llvm.org/D55009

Files:
  lib/Transforms/Scalar/GVN.cpp
  test/Transforms/GVN/PRE/pre-gep-load.ll


Index: test/Transforms/GVN/PRE/pre-gep-load.ll
===================================================================
--- test/Transforms/GVN/PRE/pre-gep-load.ll
+++ test/Transforms/GVN/PRE/pre-gep-load.ll
@@ -32,12 +32,12 @@
 ; 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]]
Index: lib/Transforms/Scalar/GVN.cpp
===================================================================
--- lib/Transforms/Scalar/GVN.cpp
+++ lib/Transforms/Scalar/GVN.cpp
@@ -2156,6 +2156,16 @@
   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())


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D55009.175696.patch
Type: text/x-patch
Size: 2113 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20181128/27de1b34/attachment.bin>