<div dir="ltr"><div class="gmail_quote" style="font-size:12.8000001907349px"><div>The right solution to this problem depends on why it was hardcoded to '6' in the first place.</div><div>Right now, in -O2, -early-cse replaces a load with a store, too early for -instcombine to recognize </div><div>subsequent duplicate loads as actual duplicates to be removed.</div><div><br></div><div>For example, consider the following code snippet:</div><div>---------------------------------------------------------------------------------------------------</div><div><div> %i = alloca %struct.A, align 4</div><div> %j = alloca %struct.A, align 4</div><div> %0 = bitcast %struct.A* %i to i8*</div><div> call void @llvm.lifetime.start(i64 4, i8* %0) #3</div><div> %call = call i32 @_Z3onev()</div><div> %a2.i = getelementptr inbounds %struct.A, %struct.A* %i, i64 0, i32 0</div><div> <b>store i32 %call, i32* %a2.i, align 4, !tbaa !1</b></div><div> %1 = bitcast %struct.A* %i to i8*</div><div> %2 = call {}* @llvm.invariant.start(i64 4, i8* %1) #3</div><div> %3 = bitcast %struct.A* %j to i8*</div><div> call void @llvm.lifetime.start(i64 4, i8* %3) #3</div><div> %4 = getelementptr inbounds %struct.A, %struct.A* %i, i64 0, i32 0</div><div> %5 = getelementptr inbounds %struct.A, %struct.A* %j, i64 0, i32 0</div><div> <b>%6 = load i32, i32* %4, align 4</b></div><div> store i32 %6, i32* %5, align 4, !tbaa !6</div><div> %7 = bitcast %struct.A* %j to i8*</div><div> %8 = call {}* @llvm.invariant.start(i64 4, i8* %7) #3</div><div> call void @_Z3bar1A(i32 %6)</div><div> call void @_Z3bar1A(i32 %6)</div><div> call void @_Z4foo2PK1AS1_(%struct.A* nonnull %j, %struct.A* nonnull %i)</div><div> %9 = getelementptr inbounds %struct.A, %struct.A* %i, i64 0, i32 0</div><div> <b>%10 = load i32, i32* %9, align 4 ; <--- duplicate. Should be removed.</b></div><div> call void @_Z3bar1A(i32 %10)</div></div>---------------------------------------------------------------------------------------------------</div><div class="gmail_quote" style="font-size:12.8000001907349px"><br></div><div class="gmail_quote" style="font-size:12.8000001907349px">After -early-cse, the above becomes<div>---------------------------------------------------------------------------------------------------</div><div><div> %i = alloca %struct.A, align 4</div><div> %j = alloca %struct.A, align 4</div><div> %0 = bitcast %struct.A* %i to i8*</div><div> call void @llvm.lifetime.start(i64 4, i8* %0) #3</div><div> %call = call i32 @_Z3onev()</div><div> %a2.i = getelementptr inbounds %struct.A, %struct.A* %i, i64 0, i32 0</div><div> <b>store i32 %call, i32* %a2.i, align 4, !tbaa !1</b></div><div> %1 = call {}* @llvm.invariant.start(i64 4, i8* %0) #3</div><div> %2 = bitcast %struct.A* %j to i8*</div><div> call void @llvm.lifetime.start(i64 4, i8* %2) #3</div><div> %3 = getelementptr inbounds %struct.A, %struct.A* %j, i64 0, i32 0</div><div> store i32 %call, i32* %3, align 4, !tbaa !6</div><div> %4 = call {}* @llvm.invariant.start(i64 4, i8* %2) #3</div><div> call void @_Z3bar1A(i32 %call)</div><div> call void @_Z3bar1A(i32 %call)</div><div> call void @_Z4foo2PK1AS1_(%struct.A* nonnull %j, %struct.A* nonnull %i)</div><div> <b>%5 = load i32, i32* %a2.i, align 4</b><b> ; <--- duplicate. Should be removed.</b></div><div> call void @_Z3bar1A(i32 %5)</div></div>---------------------------------------------------------------------------------------------------</div><div class="gmail_quote" style="font-size:12.8000001907349px">where the first load from %i has been replaced with the store into %a2.i which points to %i.</div><div class="gmail_quote" style="font-size:12.8000001907349px"><br></div><div class="gmail_quote" style="font-size:12.8000001907349px">For -instcombine to remove the duplicate load above, either</div><div class="gmail_quote" style="font-size:12.8000001907349px">* -early-cse should not merge the first load into the store -- thereby treating the load as not a "trivially redundant instruction", or</div><div class="gmail_quote" style="font-size:12.8000001907349px">* -instcombine should allow FindAvailableLoadedValue() to scan more than 6 instructions.</div><div class="gmail_quote" style="font-size:12.8000001907349px">Note that @llvm.invariant.start() calls are ignored in the count, just like @llvm.lifetime.start().</div><div class="gmail_quote" style="font-size:12.8000001907349px"><br></div><div class="gmail_quote" style="font-size:12.8000001907349px">A quick run has found the value of 8 to be the minimal needed, for this sample case scenario.</div><div class="gmail_quote" style="font-size:12.8000001907349px"><br></div><div class="gmail_quote" style="font-size:12.8000001907349px">Does this answer your question?</div><div class="gmail_quote" style="font-size:12.8000001907349px"><br></div><div class="gmail_quote" style="font-size:12.8000001907349px">Thanks,</div><div class="gmail_quote" style="font-size:12.8000001907349px">-- Larisse.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Sep 15, 2015 at 11:09 AM, Larisse Voufo <span dir="ltr"><<a href="mailto:lvoufo@google.com" target="_blank">lvoufo@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Tue, Sep 15, 2015 at 10:59 AM, Daniel Berlin <span dir="ltr"><<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Why did this get submitted without code review?<br></blockquote><div><br></div></span><div>By "this", which one of the following do you mean?</div><div>1. the refactoring in r247497 (which does nothing but consolidates occurrences of '6' into a the constant <span style="color:rgb(80,0,80)">DEF_MAX_INSTS_TO_SCAN)</span></div><div>2. the hardcoding in <span style="color:rgb(80,0,80)">r60148.</span></div><div><div class="h5"><div><span style="color:rgb(80,0,80)"><br></span></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span><br>
<br>
On Fri, Sep 11, 2015 at 6:41 PM, Larisse Voufo via llvm-commits<br>
<<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br>
</span><div><div>> Author: lvoufo<br>
> Date: Fri Sep 11 20:41:55 2015<br>
> New Revision: 247497<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=247497&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=247497&view=rev</a><br>
> Log:<br>
> Clean up: Refactoring the hardcoded value of 6 for FindAvailableLoadedValue()'s parameter MaxInstsToScan.<br>
><br>
> Modified:<br>
> llvm/trunk/include/llvm/Analysis/Loads.h<br>
> llvm/trunk/lib/Analysis/Lint.cpp<br>
> llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp<br>
> llvm/trunk/lib/Transforms/Scalar/JumpThreading.cpp<br>
><br>
> Modified: llvm/trunk/include/llvm/Analysis/Loads.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Analysis/Loads.h?rev=247497&r1=247496&r2=247497&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Analysis/Loads.h?rev=247497&r1=247496&r2=247497&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/include/llvm/Analysis/Loads.h (original)<br>
> +++ llvm/trunk/include/llvm/Analysis/Loads.h Fri Sep 11 20:41:55 2015<br>
> @@ -29,15 +29,28 @@ class MDNode;<br>
> bool isSafeToLoadUnconditionally(Value *V, Instruction *ScanFrom,<br>
> unsigned Align);<br>
><br>
> +/// DEF_MAX_INSTS_TO_SCAN - the default number of maximum instructions<br>
> +/// to scan in the block, used by FindAvailableLoadedValue().<br>
> +/// FindAvailableLoadedValue() was introduced in r60148, to improve jump<br>
> +/// threading in part by eliminating partially redundant loads.<br>
> +/// At that point, the value of MaxInstsToScan was already set to '6'<br>
> +/// without documented explanation.<br>
> +/// FIXME: Ask r60148 author for details, and complete this documentation.<br>
> +/// NOTE: As of now, every use of FindAvailableLoadedValue() uses this default<br>
> +/// value of '6'.<br>
> +#ifndef DEF_MAX_INSTS_TO_SCAN<br>
> +#define DEF_MAX_INSTS_TO_SCAN 6<br>
> +#endif<br>
> +<br>
> /// FindAvailableLoadedValue - Scan the ScanBB block backwards (starting at<br>
> /// the instruction before ScanFrom) checking to see if we have the value at<br>
> /// the memory address *Ptr locally available within a small number of<br>
> /// instructions. If the value is available, return it.<br>
> ///<br>
> -/// If not, return the iterator for the last validated instruction that the<br>
> +/// If not, return the iterator for the last validated instruction that the<br>
> /// value would be live through. If we scanned the entire block and didn't<br>
> /// find something that invalidates *Ptr or provides it, ScanFrom would be<br>
> -/// left at begin() and this returns null. ScanFrom could also be left<br>
> +/// left at begin() and this returns null. ScanFrom could also be left<br>
> ///<br>
> /// MaxInstsToScan specifies the maximum instructions to scan in the block.<br>
> /// If it is set to 0, it will scan the whole block. You can also optionally<br>
> @@ -48,7 +61,7 @@ bool isSafeToLoadUnconditionally(Value *<br>
> /// is found, it is left unmodified.<br>
> Value *FindAvailableLoadedValue(Value *Ptr, BasicBlock *ScanBB,<br>
> BasicBlock::iterator &ScanFrom,<br>
> - unsigned MaxInstsToScan = 6,<br>
> + unsigned MaxInstsToScan = DEF_MAX_INSTS_TO_SCAN,<br>
> AliasAnalysis *AA = nullptr,<br>
> AAMDNodes *AATags = nullptr);<br>
><br>
><br>
> Modified: llvm/trunk/lib/Analysis/Lint.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/Lint.cpp?rev=247497&r1=247496&r2=247497&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/Lint.cpp?rev=247497&r1=247496&r2=247497&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/lib/Analysis/Lint.cpp (original)<br>
> +++ llvm/trunk/lib/Analysis/Lint.cpp Fri Sep 11 20:41:55 2015<br>
> @@ -829,8 +829,9 @@ Value *Lint::findValueImpl(Value *V, boo<br>
> for (;;) {<br>
> if (!VisitedBlocks.insert(BB).second)<br>
> break;<br>
> - if (Value *U = FindAvailableLoadedValue(L->getPointerOperand(),<br>
> - BB, BBI, 6, AA))<br>
> + if (Value *U =<br>
> + FindAvailableLoadedValue(L->getPointerOperand(),<br>
> + BB, BBI, DEF_MAX_INSTS_TO_SCAN, AA))<br>
> return findValueImpl(U, OffsetOk, Visited);<br>
> if (BBI != BB->begin()) break;<br>
> BB = BB->getUniquePredecessor();<br>
><br>
> Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp?rev=247497&r1=247496&r2=247497&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp?rev=247497&r1=247496&r2=247497&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp (original)<br>
> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp Fri Sep 11 20:41:55 2015<br>
> @@ -750,8 +750,9 @@ Instruction *InstCombiner::visitLoadInst<br>
> // separated by a few arithmetic operations.<br>
> BasicBlock::iterator BBI = &LI;<br>
> AAMDNodes AATags;<br>
> - if (Value *AvailableVal = FindAvailableLoadedValue(Op, LI.getParent(), BBI,<br>
> - 6, AA, &AATags)) {<br>
> + if (Value *AvailableVal =<br>
> + FindAvailableLoadedValue(Op, LI.getParent(), BBI,<br>
> + DEF_MAX_INSTS_TO_SCAN, AA, &AATags)) {<br>
> if (LoadInst *NLI = dyn_cast<LoadInst>(AvailableVal)) {<br>
> unsigned KnownIDs[] = {<br>
> LLVMContext::MD_tbaa,<br>
> @@ -822,7 +823,7 @@ Instruction *InstCombiner::visitLoadInst<br>
> }<br>
><br>
> // load (select (cond, null, P)) -> load P<br>
> - if (isa<ConstantPointerNull>(SI->getOperand(1)) &&<br>
> + if (isa<ConstantPointerNull>(SI->getOperand(1)) &&<br>
> LI.getPointerAddressSpace() == 0) {<br>
> LI.setOperand(0, SI->getOperand(2));<br>
> return &LI;<br>
><br>
> Modified: llvm/trunk/lib/Transforms/Scalar/JumpThreading.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/JumpThreading.cpp?rev=247497&r1=247496&r2=247497&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/JumpThreading.cpp?rev=247497&r1=247496&r2=247497&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/lib/Transforms/Scalar/JumpThreading.cpp (original)<br>
> +++ llvm/trunk/lib/Transforms/Scalar/JumpThreading.cpp Fri Sep 11 20:41:55 2015<br>
> @@ -769,7 +769,7 @@ bool JumpThreading::ProcessBlock(BasicBl<br>
> // If we're branching on a conditional, LVI might be able to determine<br>
> // it's value at the branch instruction. We only handle comparisons<br>
> // against a constant at this time.<br>
> - // TODO: This should be extended to handle switches as well.<br>
> + // TODO: This should be extended to handle switches as well.<br>
> BranchInst *CondBr = dyn_cast<BranchInst>(BB->getTerminator());<br>
> Constant *CondConst = dyn_cast<Constant>(CondCmp->getOperand(1));<br>
> if (CondBr && CondConst && CondBr->isConditional()) {<br>
> @@ -877,7 +877,7 @@ bool JumpThreading::SimplifyPartiallyRed<br>
> BasicBlock::iterator BBIt = LI;<br>
><br>
> if (Value *AvailableVal =<br>
> - FindAvailableLoadedValue(LoadedPtr, LoadBB, BBIt, 6)) {<br>
> + FindAvailableLoadedValue(LoadedPtr, LoadBB, BBIt)) {<br>
> // If the value if the load is locally available within the block, just use<br>
> // it. This frequently occurs for reg2mem'd allocas.<br>
> //cerr << "LOAD ELIMINATED:\n" << *BBIt << *LI << "\n";<br>
> @@ -922,7 +922,8 @@ bool JumpThreading::SimplifyPartiallyRed<br>
> // Scan the predecessor to see if the value is available in the pred.<br>
> BBIt = PredBB->end();<br>
> AAMDNodes ThisAATags;<br>
> - Value *PredAvailable = FindAvailableLoadedValue(LoadedPtr, PredBB, BBIt, 6,<br>
> + Value *PredAvailable = FindAvailableLoadedValue(LoadedPtr, PredBB, BBIt,<br>
> + DEF_MAX_INSTS_TO_SCAN,<br>
> nullptr, &ThisAATags);<br>
> if (!PredAvailable) {<br>
> OneUnavailablePred = PredBB;<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div></div></div><br></div></div>
</blockquote></div><br></div>