[PATCH] D19463: Thread TargetLibraryInfo through PHITransAddr.

[Nick Lewycky via llvm-commits]

Rafael Espíndola wrote:
> Why not just delete the field instead?

The field is used, PHITransAddr is passing it along to other functions. 
Presumably those functions could use it to power their optimizations.

What PHITransAddr has to do is find an existing Value which represents 
the value through one branch of a PHI. This lines up really well with 
instruction simplify's API of always returning an existing value (or a 
constant). For whatever reason, instruction simplify finds it useful to 
take TLI through all its interfaces. I see no reason to refuse to give 
it a TLI when the originally requesting pass had a TLI available.

If we delete it then someone might fix a future missed optimization by 
teaching it to the caller of PHITransAddr, such as GVN, rather than 
relying on a single implementation in instruction simplify to get it 
correct for them.

Nick

> On Apr 24, 2016 2:08 AM, "Nick Lewycky via llvm-commits"
> <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>
>     nicholas created this revision.
>     nicholas added a subscriber: llvm-commits.
>     nicholas set the repository for this revision to rL LLVM.
>
>     PHITransAddr has a TargetLibraryInfo* member which it always sets to
>     nullptr at construction. It has the logic to pass the TLI down to
>     instruction simplify utility functions, but it doesn't take any
>     value from the pass.
>
>     The attached pass simply adds the plumbing to the four callees,
>     three of which have TLIs to pass in.
>
>     There is no testcase included, all the existing tests continue to
>     pass. This change wasn't motivated by any example code, only by the
>     "huh that's funny" of seeing the PHITransAddr constructor always set
>     TLI to nullptr.
>
>     Repository:
>        rL LLVM
>
>     http://reviews.llvm.org/D19463
>
>     Files:
>        include/llvm/Analysis/PHITransAddr.h
>        include/llvm/Transforms/Utils/MemorySSA.h
>        lib/Analysis/MemoryDependenceAnalysis.cpp
>        lib/Analysis/PHITransAddr.cpp
>        lib/Transforms/Scalar/GVN.cpp
>
>     Index: lib/Transforms/Scalar/GVN.cpp
>     ===================================================================
>     --- lib/Transforms/Scalar/GVN.cpp
>     +++ lib/Transforms/Scalar/GVN.cpp
>     @@ -1494,7 +1494,7 @@
>           // If all preds have a single successor, then we know it is
>     safe to insert
>           // the load on the pred (?!?), so we can insert code to
>     materialize the
>           // pointer if it is not available.
>     -    PHITransAddr Address(LI->getPointerOperand(), DL, AC);
>     +    PHITransAddr Address(LI->getPointerOperand(), DL, TLI, AC);
>           Value *LoadPtr = nullptr;
>           LoadPtr = Address.PHITranslateWithInsertion(LoadBB,
>     UnavailablePred,
>                                                       *DT, NewInsts);
>     Index: lib/Analysis/MemoryDependenceAnalysis.cpp
>     ===================================================================
>     --- lib/Analysis/MemoryDependenceAnalysis.cpp
>     +++ lib/Analysis/MemoryDependenceAnalysis.cpp
>     @@ -909,7 +909,7 @@
>           return;
>         }
>         const DataLayout &DL = FromBB->getModule()->getDataLayout();
>     -  PHITransAddr Address(const_cast<Value *>(Loc.Ptr), DL, &AC);
>     +  PHITransAddr Address(const_cast<Value *>(Loc.Ptr), DL, &TLI, &AC);
>
>         // This is the set of blocks we've inspected, and the pointer we
>     consider in
>         // each block.  Because of critical edges, we currently bail out
>     if querying
>     Index: lib/Analysis/PHITransAddr.cpp
>     ===================================================================
>     --- lib/Analysis/PHITransAddr.cpp
>     +++ lib/Analysis/PHITransAddr.cpp
>     @@ -371,7 +371,7 @@
>                                  SmallVectorImpl<Instruction*> &NewInsts) {
>         // See if we have a version of this value already available and
>     dominating
>         // PredBB.  If so, there is no need to insert a new instance of it.
>     -  PHITransAddr Tmp(InVal, DL, AC);
>     +  PHITransAddr Tmp(InVal, DL, TLI, AC);
>         if (!Tmp.PHITranslateValue(CurBB, PredBB, &DT,
>     /*MustDominate=*/true))
>           return Tmp.getAddr();
>
>     Index: include/llvm/Transforms/Utils/MemorySSA.h
>     ===================================================================
>     --- include/llvm/Transforms/Utils/MemorySSA.h
>     +++ include/llvm/Transforms/Utils/MemorySSA.h
>     @@ -918,7 +918,8 @@
>           if (WalkingPhi && Location.Ptr) {
>             PHITransAddr Translator(
>                 const_cast<Value *>(Location.Ptr),
>     -          OriginalAccess->getBlock()->getModule()->getDataLayout(),
>     nullptr);
>     +          OriginalAccess->getBlock()->getModule()->getDataLayout(),
>     nullptr,
>     +          nullptr);
>             if (!Translator.PHITranslateValue(OriginalAccess->getBlock(),
>
>       DefIterator.getPhiArgBlock(), nullptr,
>                                               false))
>     Index: include/llvm/Analysis/PHITransAddr.h
>     ===================================================================
>     --- include/llvm/Analysis/PHITransAddr.h
>     +++ include/llvm/Analysis/PHITransAddr.h
>     @@ -50,8 +50,9 @@
>         SmallVector<Instruction*, 4> InstInputs;
>
>       public:
>     -  PHITransAddr(Value *addr, const DataLayout &DL, AssumptionCache *AC)
>     -      : Addr(addr), DL(DL), TLI(nullptr), AC(AC) {
>     +  PHITransAddr(Value *addr, const DataLayout &DL, const
>     TargetLibraryInfo *TLI,
>     +               AssumptionCache *AC)
>     +      : Addr(addr), DL(DL), TLI(TLI), AC(AC) {
>           // If the address is an instruction, the whole thing is
>     considered an input.
>           if (Instruction *I = dyn_cast<Instruction>(Addr))
>             InstInputs.push_back(I);
>
>
>
>     _______________________________________________
>     llvm-commits mailing list
>     llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
>     http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>