[llvm-commits] [PATCH] Fix a miscompile in memory dependency analysis
Manman Ren
mren at apple.com
Fri Dec 21 13:20:34 PST 2012
Ping
On Dec 20, 2012, at 8:03 AM, Manman Ren wrote:
> Ping
>
> On Dec 19, 2012, at 10:05 AM, Manman Ren <mren at apple.com> wrote:
>
>>
>>
>> Thanks for having the detailed algorithm.
>> According to Chandler (and I agree),
>>
>> We can't rely on LoopInfo to describe all backedges as LoopInfo doesn't represent unnatural loops -- essentially it isn't guaranteed to find all cycles.
>>
>> I now have a simple implementation of a very conservative reachablity analysis:
>> + // Conservatively return true. Return false, if there is a single path
>> + // starting from "From" and the path does not reach "To".
>> + static bool hasPath(const BasicBlock *From, const BasicBlock *To) {
>> + const unsigned MaxCheck = 5;
>> + const BasicBlock *Current = From;
>> + for (unsigned I = 0; I < MaxCheck; I++) {
>> + unsigned NumSuccs = Current->getTerminator()->getNumSuccessors();
>> + if (NumSuccs > 1)
>> + return true;
>> + if (NumSuccs == 0)
>> + return false;
>> + Current = Current->getTerminator()->getSuccessor(0);
>> + if (Current == To)
>> + return true;
>> + }
>> + return true;
>> + }
>>
>> And I will file a PR to request a more advanced reachability analysis.
>> Please review the attached patch,
>>
>> Thanks,
>> Manman
>>
>> <mem_dep_2.patch>
>>
>> On Dec 19, 2012, at 2:07 AM, Nick Lewycky wrote:
>>
>>> Nick Lewycky wrote:
>>>> Manman Ren wrote:
>>>>>
>>>>> Given input:
>>>>> while.body:
>>>>> call void @RunInMode(i32 100) nounwind
>>>>> %1 = load i32* %tasksIdle, align 4
>>>>> %tobool = icmp eq i32 %1, 0
>>>>> br i1 %tobool, label %while.cond.backedge, label %if.then
>>>>>
>>>>> if.then:
>>>>> store i32 0, i32* %tasksIdle, align 4
>>>>> call void @TimerCreate(i32* %shouldExit) nounwind
>>>>> br label %while.cond.backedge
>>>>>
>>>>> while.cond.backedge:
>>>>> %2 = load i32* %shouldExit, align 4
>>>>> %cmp = icmp eq i32 %2, 0
>>>>> br i1 %cmp, label %while.body, label %while.cond.while.end_crit_edge
>>>>>
>>>>> Make sure we are not returning "call RunInMode" does not modify
>>>>> %shouldExit.
>>>>> Currently, we optimize this into
>>>>> while.cond.backedge: ; preds = %if.then, %while.body
>>>>> %1 = phi i32 [ %.pre, %if.then ], [ 0, %while.body ]
>>>>> %cmp = icmp eq i32 %1, 0
>>>>> br i1 %cmp, label %while.body, label %while.cond.while.end_crit_edge
>>>>>
>>>>> The proper fix is to check all uses of %shouldExit as long as there is
>>>>> a possible
>>>>> path from the use to "call RunInMode". The proposed patch uses
>>>>> LoopInfo to approximate the reachability analysis.
>>>>> Suggestions on how to approximate this are welcome.
>>>>>
>>>>> Please review, Thanks,
>>>>
>>>> Thanks for working on this.
>>>>
>>>> I think you should factor out logic for "is Inst2 reachable from Inst1"
>>>> and put it somewhere reusable (BasicBlockUtils?). Making it take
>>>> DominatorTree* and LoopInfo* is the right idea, but we may not have
>>>> those available. If the method requires one of those, it should take it
>>>> as a reference instead of a pointer.
>>>>
>>>> + AU.addRequired<LoopInfo>();
>>>>
>>>> Sorry, I don't think we can make MemoryDependenceAnalysis depend on
>>>> LoopInfo.
>>>>
>>>> What we can do is check if it just happens to be available, due to the
>>>> pass ordering we happen to be running in. If it is,
>>>> getAnalysisIfAvailable<LoopInfo>() will return the pointer and we can
>>>> use it to speed up the search, or NULL.
>>>>
>>>> I spent a while thinking about whether LoopInfo ever actually helps us
>>>> prune the search, both in the case where we have a dominator tree
>>>> available and when we don't. I decided it does (in both cases), and I
>>>> wanted to quickly jot down what I think the algorithm is:
>>>>
>>>> We're searching for whether there exists any path from A to B.
>>>> - if we have loop info and not dominator trees, set B = B's loop header
>>>> (this is an optimization only).
>>>> - push A on worklist.
>>>> - pop A' off the worklist and
>>>> - if A' and B are not both in the same loop, then A' is a dead end. Skip
>>>> to the next item on the worklist.
>>>> - if A' dominates B, we have found a path. Stop.
>>>> - push all of A''s successors onto the worklist, except the ones we've
>>>> visited before.
>>>> - if the worklist is empty, there is no path. Stop.
>>>
>>> Ignore this. It's both subtly wrong and rather inefficient.
>>>
>>> - push A on the worklist.
>>> - pop A' off the worklist and
>>> - if there is a loop that contains both A' and B, then there is a path. Stop.
>>> - if A' dominates B, there is a path. Stop.
>>> - if A' is in a loop (and we have loop info), add the outermost loop's exit blocks to the worklist; else (if A' is not in a loop) add A''s successors to the worklist.
>>> - if the worklist is empty, there is no path. Stop.
>>>
>>> Nick
>>>
>>>> Note that "in the same loop" can't be implemented by a simple
>>>> getLoopFor(A) == getLoopFor(B), because of nested loops. The test we
>>>> really want is "does there exist a Loop* that contains both blocks?".
>>>>
>>>> If the dominator tree is not available then the test for "A' dominates
>>>> B" becomes "A' == B" with no change in precision. If the loop
>>>> information is not available, then we skip that test, again with loss of
>>>> precision. We should have some sort of visited-blocks limit to make sure
>>>> this doesn't take too much compile time.
>>>>
>>>> Nick
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>
>>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121221/d4700b31/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mem_dep_2.patch
Type: application/octet-stream
Size: 4784 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121221/d4700b31/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121221/d4700b31/attachment-0001.html>
More information about the llvm-commits
mailing list