[llvm] f8919d2 - [NFC][GVN] Put phi-translation of 'add' behind a switch

[Peter Waller via llvm-commits]

Author: Peter Waller
Date: 2022-07-25T07:59:47Z
New Revision: f8919d2f7ebaf89074a075d460ab16c02fb72df0

URL: https://github.com/llvm/llvm-project/commit/f8919d2f7ebaf89074a075d460ab16c02fb72df0
DIFF: https://github.com/llvm/llvm-project/commit/f8919d2f7ebaf89074a075d460ab16c02fb72df0.diff

LOG: [NFC][GVN] Put phi-translation of 'add' behind a switch

The code in this `#if 0` block appears to be a net benefit. Put it
behind a switch defaulting to off to support experimentation and as a
request for comment.

The codegen impact of enabling this that I'm currently persuing is that
it allows PRE to take place more frequently, particularly in loops with
second order recurrences.

Preliminary experimental data:

Across LNT on AArch64, 54 benchmarks are sped up by >1%, and 42 are
regressed by >1%, the geomean (exec_time_enabled / exec_time_disabled)
of these 96 "1% or greater significance" benchmarks is 0.991. For the
full set of 770 benchmarks it's 0.998.

There are two benchmarks which experience a >30% speedup, and the worst
slowdown is ~12%, and for every benchmark with a slowdown there is a
benckmark which is sped up by a greater factor.

Differential Revision: https://reviews.llvm.org/D130241

Added: 
    llvm/test/Transforms/GVN/PRE/phi-translate-add.ll

Modified: 
    llvm/lib/Analysis/PHITransAddr.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Analysis/PHITransAddr.cpp b/llvm/lib/Analysis/PHITransAddr.cpp
index 7571bd0059cc6..5b0fbca238911 100644
--- a/llvm/lib/Analysis/PHITransAddr.cpp
+++ b/llvm/lib/Analysis/PHITransAddr.cpp
@@ -21,6 +21,10 @@
 #include "llvm/Support/raw_ostream.h"
 using namespace llvm;
 
+static cl::opt<bool> EnableAddPhiTranslation(
+    "gvn-add-phi-translation", cl::init(false), cl::Hidden,
+    cl::desc("Enable phi-translation of add instructions"));
+
 static bool CanPHITrans(Instruction *Inst) {
   if (isa<PHINode>(Inst) ||
       isa<GetElementPtrInst>(Inst))
@@ -410,14 +414,14 @@ InsertPHITranslatedSubExpr(Value *InVal, BasicBlock *CurBB,
     return Result;
   }
 
-#if 0
-  // FIXME: This code works, but it is unclear that we actually want to insert
-  // a big chain of computation in order to make a value available in a block.
-  // This needs to be evaluated carefully to consider its cost trade offs.
-
   // Handle add with a constant RHS.
-  if (Inst->getOpcode() == Instruction::Add &&
+  if (EnableAddPhiTranslation && Inst->getOpcode() == Instruction::Add &&
       isa<ConstantInt>(Inst->getOperand(1))) {
+
+    // FIXME: This code works, but it is unclear that we actually want to insert
+    // a big chain of computation in order to make a value available in a block.
+    // This needs to be evaluated carefully to consider its cost trade offs.
+
     // PHI translate the LHS.
     Value *OpVal = InsertPHITranslatedSubExpr(Inst->getOperand(0),
                                               CurBB, PredBB, DT, NewInsts);
@@ -431,7 +435,6 @@ InsertPHITranslatedSubExpr(Value *InVal, BasicBlock *CurBB,
     NewInsts.push_back(Res);
     return Res;
   }
-#endif
 
   return nullptr;
 }

diff  --git a/llvm/test/Transforms/GVN/PRE/phi-translate-add.ll b/llvm/test/Transforms/GVN/PRE/phi-translate-add.ll
new file mode 100644
index 0000000000000..e869175708d17
--- /dev/null
+++ b/llvm/test/Transforms/GVN/PRE/phi-translate-add.ll
@@ -0,0 +1,44 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -gvn -gvn-add-phi-translation=true  -S < %s | FileCheck %s --check-prefix=ADD-TRANS-ON
+; RUN: opt -gvn -gvn-add-phi-translation=false -S < %s | FileCheck %s --check-prefix=ADD-TRANS-OFF
+
+; Test that phi translation is able to hoist a load whose address
+; depends on an add also being hoisted.
+define double @phi_translation_hoists_add(ptr %a, i64 %idx) {
+; ADD-TRANS-ON-LABEL: @phi_translation_hoists_add(
+; ADD-TRANS-ON-NEXT:  entry:
+; ADD-TRANS-ON-NEXT:    [[ADD_PHI_TRANS_INSERT:%.*]] = add nuw nsw i64 [[IDX:%.*]], 1
+; ADD-TRANS-ON-NEXT:    [[GEP_PHI_TRANS_INSERT:%.*]] = getelementptr inbounds double, ptr [[A:%.*]], i64 [[ADD_PHI_TRANS_INSERT]]
+; ADD-TRANS-ON-NEXT:    [[LOAD_PRE:%.*]] = load double, ptr [[GEP_PHI_TRANS_INSERT]], align 8
+; ADD-TRANS-ON-NEXT:    br label [[FOR_BODY:%.*]]
+; ADD-TRANS-ON:       for.body:
+; ADD-TRANS-ON-NEXT:    [[CMP:%.*]] = fcmp ole double [[LOAD_PRE]], 1.000000e+00
+; ADD-TRANS-ON-NEXT:    br i1 [[CMP]], label [[EXIT:%.*]], label [[FOR_BODY]]
+; ADD-TRANS-ON:       exit:
+; ADD-TRANS-ON-NEXT:    ret double [[LOAD_PRE]]
+;
+; ADD-TRANS-OFF-LABEL: @phi_translation_hoists_add(
+; ADD-TRANS-OFF-NEXT:  entry:
+; ADD-TRANS-OFF-NEXT:    br label [[FOR_BODY:%.*]]
+; ADD-TRANS-OFF:       for.body:
+; ADD-TRANS-OFF-NEXT:    [[ADD:%.*]] = add nuw nsw i64 [[IDX:%.*]], 1
+; ADD-TRANS-OFF-NEXT:    [[GEP:%.*]] = getelementptr inbounds double, ptr [[A:%.*]], i64 [[ADD]]
+; ADD-TRANS-OFF-NEXT:    [[LOAD:%.*]] = load double, ptr [[GEP]], align 8
+; ADD-TRANS-OFF-NEXT:    [[CMP:%.*]] = fcmp ole double [[LOAD]], 1.000000e+00
+; ADD-TRANS-OFF-NEXT:    br i1 [[CMP]], label [[EXIT:%.*]], label [[FOR_BODY]]
+; ADD-TRANS-OFF:       exit:
+; ADD-TRANS-OFF-NEXT:    ret double [[LOAD]]
+;
+entry:
+  br label %for.body
+
+for.body:                                         ; preds = %for.body, %entry
+  %add = add nuw nsw i64 %idx, 1
+  %gep = getelementptr inbounds double, ptr %a, i64 %add
+  %load = load double, ptr %gep
+  %cmp = fcmp ole double %load, 1.000000e+00
+  br i1 %cmp, label %exit, label %for.body
+
+exit:
+  ret double %load
+}