[llvm] r247497 - Clean up: Refactoring the hardcoded value of 6 for FindAvailableLoadedValue()'s parameter MaxInstsToScan.

Larisse Voufo via llvm-commits llvm-commits at lists.llvm.org
Tue Sep 15 11:09:27 PDT 2015


On Tue, Sep 15, 2015 at 10:59 AM, Daniel Berlin <dberlin at dberlin.org> wrote:

> Why did this get submitted without code review?
>

By "this", which one of the following do you mean?
1. the refactoring in r247497 (which does nothing but consolidates
occurrences of '6' into a the constant DEF_MAX_INSTS_TO_SCAN)
2. the hardcoding in r60148.


>
> On Fri, Sep 11, 2015 at 6:41 PM, Larisse Voufo via llvm-commits
> <llvm-commits at lists.llvm.org> wrote:
> > Author: lvoufo
> > Date: Fri Sep 11 20:41:55 2015
> > New Revision: 247497
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=247497&view=rev
> > Log:
> > Clean up: Refactoring the hardcoded value of 6 for
> FindAvailableLoadedValue()'s parameter MaxInstsToScan.
> >
> > Modified:
> >     llvm/trunk/include/llvm/Analysis/Loads.h
> >     llvm/trunk/lib/Analysis/Lint.cpp
> >     llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
> >     llvm/trunk/lib/Transforms/Scalar/JumpThreading.cpp
> >
> > Modified: llvm/trunk/include/llvm/Analysis/Loads.h
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Analysis/Loads.h?rev=247497&r1=247496&r2=247497&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/include/llvm/Analysis/Loads.h (original)
> > +++ llvm/trunk/include/llvm/Analysis/Loads.h Fri Sep 11 20:41:55 2015
> > @@ -29,15 +29,28 @@ class MDNode;
> >  bool isSafeToLoadUnconditionally(Value *V, Instruction *ScanFrom,
> >                                   unsigned Align);
> >
> > +/// DEF_MAX_INSTS_TO_SCAN - the default number of maximum instructions
> > +/// to scan in the block, used by FindAvailableLoadedValue().
> > +/// FindAvailableLoadedValue() was introduced in r60148, to improve jump
> > +/// threading in part by eliminating partially redundant loads.
> > +/// At that point, the value of MaxInstsToScan was already set to '6'
> > +/// without documented explanation.
> > +/// FIXME: Ask r60148 author for details, and complete this
> documentation.
> > +/// NOTE: As of now, every use of FindAvailableLoadedValue() uses this
> default
> > +/// value of '6'.
> > +#ifndef DEF_MAX_INSTS_TO_SCAN
> > +#define DEF_MAX_INSTS_TO_SCAN 6
> > +#endif
> > +
> >  /// FindAvailableLoadedValue - Scan the ScanBB block backwards
> (starting at
> >  /// the instruction before ScanFrom) checking to see if we have the
> value at
> >  /// the memory address *Ptr locally available within a small number of
> >  ///  instructions. If the value is available, return it.
> >  ///
> > -/// If not, return the iterator for the last validated instruction that
> the
> > +/// If not, return the iterator for the last validated instruction that
> the
> >  /// value would be live through.  If we scanned the entire block and
> didn't
> >  /// find something that invalidates *Ptr or provides it, ScanFrom would
> be
> > -/// left at begin() and this returns null.  ScanFrom could also be left
> > +/// left at begin() and this returns null.  ScanFrom could also be left
> >  ///
> >  /// MaxInstsToScan specifies the maximum instructions to scan in the
> block.
> >  /// If it is set to 0, it will scan the whole block. You can also
> optionally
> > @@ -48,7 +61,7 @@ bool isSafeToLoadUnconditionally(Value *
> >  /// is found, it is left unmodified.
> >  Value *FindAvailableLoadedValue(Value *Ptr, BasicBlock *ScanBB,
> >                                  BasicBlock::iterator &ScanFrom,
> > -                                unsigned MaxInstsToScan = 6,
> > +                                unsigned MaxInstsToScan =
> DEF_MAX_INSTS_TO_SCAN,
> >                                  AliasAnalysis *AA = nullptr,
> >                                  AAMDNodes *AATags = nullptr);
> >
> >
> > Modified: llvm/trunk/lib/Analysis/Lint.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/Lint.cpp?rev=247497&r1=247496&r2=247497&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/lib/Analysis/Lint.cpp (original)
> > +++ llvm/trunk/lib/Analysis/Lint.cpp Fri Sep 11 20:41:55 2015
> > @@ -829,8 +829,9 @@ Value *Lint::findValueImpl(Value *V, boo
> >      for (;;) {
> >        if (!VisitedBlocks.insert(BB).second)
> >          break;
> > -      if (Value *U = FindAvailableLoadedValue(L->getPointerOperand(),
> > -                                              BB, BBI, 6, AA))
> > +      if (Value *U =
> > +          FindAvailableLoadedValue(L->getPointerOperand(),
> > +                                   BB, BBI, DEF_MAX_INSTS_TO_SCAN, AA))
> >          return findValueImpl(U, OffsetOk, Visited);
> >        if (BBI != BB->begin()) break;
> >        BB = BB->getUniquePredecessor();
> >
> > Modified:
> llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp?rev=247497&r1=247496&r2=247497&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
> (original)
> > +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
> Fri Sep 11 20:41:55 2015
> > @@ -750,8 +750,9 @@ Instruction *InstCombiner::visitLoadInst
> >    // separated by a few arithmetic operations.
> >    BasicBlock::iterator BBI = &LI;
> >    AAMDNodes AATags;
> > -  if (Value *AvailableVal = FindAvailableLoadedValue(Op,
> LI.getParent(), BBI,
> > -                                                     6, AA, &AATags)) {
> > +  if (Value *AvailableVal =
> > +      FindAvailableLoadedValue(Op, LI.getParent(), BBI,
> > +                               DEF_MAX_INSTS_TO_SCAN, AA, &AATags)) {
> >      if (LoadInst *NLI = dyn_cast<LoadInst>(AvailableVal)) {
> >        unsigned KnownIDs[] = {
> >          LLVMContext::MD_tbaa,
> > @@ -822,7 +823,7 @@ Instruction *InstCombiner::visitLoadInst
> >        }
> >
> >        // load (select (cond, null, P)) -> load P
> > -      if (isa<ConstantPointerNull>(SI->getOperand(1)) &&
> > +      if (isa<ConstantPointerNull>(SI->getOperand(1)) &&
> >            LI.getPointerAddressSpace() == 0) {
> >          LI.setOperand(0, SI->getOperand(2));
> >          return &LI;
> >
> > Modified: llvm/trunk/lib/Transforms/Scalar/JumpThreading.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/JumpThreading.cpp?rev=247497&r1=247496&r2=247497&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/lib/Transforms/Scalar/JumpThreading.cpp (original)
> > +++ llvm/trunk/lib/Transforms/Scalar/JumpThreading.cpp Fri Sep 11
> 20:41:55 2015
> > @@ -769,7 +769,7 @@ bool JumpThreading::ProcessBlock(BasicBl
> >      // If we're branching on a conditional, LVI might be able to
> determine
> >      // it's value at the branch instruction.  We only handle comparisons
> >      // against a constant at this time.
> > -    // TODO: This should be extended to handle switches as well.
> > +    // TODO: This should be extended to handle switches as well.
> >      BranchInst *CondBr = dyn_cast<BranchInst>(BB->getTerminator());
> >      Constant *CondConst = dyn_cast<Constant>(CondCmp->getOperand(1));
> >      if (CondBr && CondConst && CondBr->isConditional()) {
> > @@ -877,7 +877,7 @@ bool JumpThreading::SimplifyPartiallyRed
> >    BasicBlock::iterator BBIt = LI;
> >
> >    if (Value *AvailableVal =
> > -        FindAvailableLoadedValue(LoadedPtr, LoadBB, BBIt, 6)) {
> > +        FindAvailableLoadedValue(LoadedPtr, LoadBB, BBIt)) {
> >      // If the value if the load is locally available within the block,
> just use
> >      // it.  This frequently occurs for reg2mem'd allocas.
> >      //cerr << "LOAD ELIMINATED:\n" << *BBIt << *LI << "\n";
> > @@ -922,7 +922,8 @@ bool JumpThreading::SimplifyPartiallyRed
> >      // Scan the predecessor to see if the value is available in the
> pred.
> >      BBIt = PredBB->end();
> >      AAMDNodes ThisAATags;
> > -    Value *PredAvailable = FindAvailableLoadedValue(LoadedPtr, PredBB,
> BBIt, 6,
> > +    Value *PredAvailable = FindAvailableLoadedValue(LoadedPtr, PredBB,
> BBIt,
> > +
> DEF_MAX_INSTS_TO_SCAN,
> >                                                      nullptr,
> &ThisAATags);
> >      if (!PredAvailable) {
> >        OneUnavailablePred = PredBB;
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150915/263ebc2f/attachment.html>


More information about the llvm-commits mailing list