[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:27:54 PDT 2015
The right solution to this problem depends on why it was hardcoded to '6'
in the first place.
Right now, in -O2, -early-cse replaces a load with a store, too early for
-instcombine to recognize
subsequent duplicate loads as actual duplicates to be removed.
For example, consider the following code snippet:
---------------------------------------------------------------------------------------------------
%i = alloca %struct.A, align 4
%j = alloca %struct.A, align 4
%0 = bitcast %struct.A* %i to i8*
call void @llvm.lifetime.start(i64 4, i8* %0) #3
%call = call i32 @_Z3onev()
%a2.i = getelementptr inbounds %struct.A, %struct.A* %i, i64 0, i32 0
*store i32 %call, i32* %a2.i, align 4, !tbaa !1*
%1 = bitcast %struct.A* %i to i8*
%2 = call {}* @llvm.invariant.start(i64 4, i8* %1) #3
%3 = bitcast %struct.A* %j to i8*
call void @llvm.lifetime.start(i64 4, i8* %3) #3
%4 = getelementptr inbounds %struct.A, %struct.A* %i, i64 0, i32 0
%5 = getelementptr inbounds %struct.A, %struct.A* %j, i64 0, i32 0
*%6 = load i32, i32* %4, align 4*
store i32 %6, i32* %5, align 4, !tbaa !6
%7 = bitcast %struct.A* %j to i8*
%8 = call {}* @llvm.invariant.start(i64 4, i8* %7) #3
call void @_Z3bar1A(i32 %6)
call void @_Z3bar1A(i32 %6)
call void @_Z4foo2PK1AS1_(%struct.A* nonnull %j, %struct.A* nonnull %i)
%9 = getelementptr inbounds %struct.A, %struct.A* %i, i64 0, i32 0
*%10 = load i32, i32* %9, align 4 ; <--- duplicate. Should be
removed.*
call void @_Z3bar1A(i32 %10)
---------------------------------------------------------------------------------------------------
After -early-cse, the above becomes
---------------------------------------------------------------------------------------------------
%i = alloca %struct.A, align 4
%j = alloca %struct.A, align 4
%0 = bitcast %struct.A* %i to i8*
call void @llvm.lifetime.start(i64 4, i8* %0) #3
%call = call i32 @_Z3onev()
%a2.i = getelementptr inbounds %struct.A, %struct.A* %i, i64 0, i32 0
*store i32 %call, i32* %a2.i, align 4, !tbaa !1*
%1 = call {}* @llvm.invariant.start(i64 4, i8* %0) #3
%2 = bitcast %struct.A* %j to i8*
call void @llvm.lifetime.start(i64 4, i8* %2) #3
%3 = getelementptr inbounds %struct.A, %struct.A* %j, i64 0, i32 0
store i32 %call, i32* %3, align 4, !tbaa !6
%4 = call {}* @llvm.invariant.start(i64 4, i8* %2) #3
call void @_Z3bar1A(i32 %call)
call void @_Z3bar1A(i32 %call)
call void @_Z4foo2PK1AS1_(%struct.A* nonnull %j, %struct.A* nonnull %i)
*%5 = load i32, i32* %a2.i, align 4** ; <--- duplicate. Should be
removed.*
call void @_Z3bar1A(i32 %5)
---------------------------------------------------------------------------------------------------
where the first load from %i has been replaced with the store into %a2.i
which points to %i.
For -instcombine to remove the duplicate load above, either
* -early-cse should not merge the first load into the store -- thereby
treating the load as not a "trivially redundant instruction", or
* -instcombine should allow FindAvailableLoadedValue() to scan more than 6
instructions.
Note that @llvm.invariant.start() calls are ignored in the count, just
like @llvm.lifetime.start().
A quick run has found the value of 8 to be the minimal needed, for this
sample case scenario.
Does this answer your question?
Thanks,
-- Larisse.
On Tue, Sep 15, 2015 at 11:09 AM, Larisse Voufo <lvoufo at google.com> wrote:
>
>
> 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/c61a49c3/attachment.html>
More information about the llvm-commits
mailing list