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

[Manman Ren]

Hi Nick,

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


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
>> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121219/e60abf13/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/20121219/e60abf13/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121219/e60abf13/attachment-0001.html>