[llvm-commits] [PATCH] Fix a miscompile in memory dependency analysis

[Nick Lewycky]

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
>