<div dir="ltr">On 13 June 2013 01:15, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<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"><div dir="ltr"><div><div>On Mon, Jun 10, 2013 at 5:24 PM, Nick Lewycky <span dir="ltr"><<a href="mailto:nlewycky@google.com" target="_blank">nlewycky@google.com</a>></span> wrote:<br>
</div></div><div class="gmail_extra"><div class="gmail_quote"><div><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"><div dir="ltr">This patch reimplements the "hasPath" utility function currently living in AliasAnalysis.cpp. There's some discussion of this algorithm and some context back in this thread from December last year:<div>
<a href="http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20121217/159680.html" target="_blank">http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20121217/159680.html</a></div><div><br>
</div><div>I also refactor it into a new home in lib/Analysis, called CFG.cpp. This is created by breaking apart lib/Transforms/Utils/BasicBlockUtils.cpp and moving everything which is there but actually only does analysis into CFG.cpp.</div>
<div><br></div><div>Please review!</div></div></blockquote><div><br></div></div></div><div>Mind putting the next version into Phab? Much easier to review there.</div></div></div></div></blockquote><div><br></div><div>
<a href="http://llvm-reviews.chandlerc.com/D996" target="_blank">http://llvm-reviews.chandlerc.com/D996</a> . This is just the same patch I sent out previously, no changes yet.<br></div><div> <br></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">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>+/// FindFunctionBackedges - Analyze the specified function to find all of the<br></div><div>
<div>+/// loop backedges in the function and return them. This is a relatively cheap</div><div>+/// (compared to computing dominators and loop info) analysis.</div><div><br></div><div>Throughout this patch, please switch to the modern form of Doxygen comments.</div>
</div></div></div></div></blockquote><div><br></div><div>This is copied verbatim from BasicBlockUtils, but done anyways.</div><div><br></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">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>
<div>+///<br></div><div>+/// The output is added to Result, as pairs of <from,to> edge info.</div><div>+void FindFunctionBackedges(const Function &F,</div><div>+ SmallVectorImpl<std::pair<const BasicBlock*,const BasicBlock*> > &Result);</div>
<div><br></div><div>Do you promise to have append semantics? If so, is that useful?</div><div><br></div><div>If not, why not return? Is this just hiding the N of the small vector, and is that a good enoguh reason to use an output parameter?</div>
<div><br></div><div>Also, throughout this patch please switch to modern naming conventions for functions.</div><div><br></div><div>Should this really be a public interface at all? Are there users other than hasPath?</div>
</div></div></div></div></blockquote><div><br></div><div>This is copied verbatim from BasicBlockUtils. hasPath doesn't even use it. It looks like the only caller is JumpThreading today.</div><div><br></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">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>
<div>+<br></div><div>+/// GetSuccessorNumber - Search for the specified successor of basic block BB</div><div>+/// and return its position in the terminator instruction's list of</div><div>+/// successors. It is an error to call this with a block that is not a</div>
<div>+/// successor.</div><div>+unsigned GetSuccessorNumber(BasicBlock *BB, BasicBlock *Succ);</div><div><br></div><div>This seems like it should stay in BBUtils... It has little to do with the CFG itself. Personally, I don't think this should be in *any* public interface unless we intend to put it into the TerminatorInst's public interface or BasicBlock's public interface.</div>
</div></div></div></div></blockquote><div><br></div><div>I was thinking the same thing -- this belongs in BasicBlock's public interface. If you want, I can commit that move ahead of this patch?</div><div><br>
</div><div>I want this patch to be smaller. The only things I want in this patch are a) new implementation of hasPath b) hasPath lives somewhere publicly accessible. There wasn't a good place for hasPath to live, so I had to create one, and for that to make sense, I had to move things into it. Putting hasPath in BasicBlockUtils doesn't work because Analysis can't depend on Transforms.</div>
<div><br></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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div>+<br></div><div>+/// isCriticalEdge - Return true if the specified edge is a critical edge.</div><div>+/// Critical edges are edges from a block with multiple successors to a block</div><div>+/// with multiple predecessors.</div>
<div>+///</div><div>+bool isCriticalEdge(const TerminatorInst *TI, unsigned SuccNum,</div><div>+ bool AllowIdenticalEdges = false);</div><div><br></div><div>Why not accept a BasicBlock* for the successor? Why not accept a pair of BasicBlock*'s much like you store in the result vector for finding backedges?</div>
</div></div></div></blockquote><div><br></div><div>Again this is copied verbatim from BBUtils but to answer your question, Terminator + successor-number identifies a single edge while pair<BasicBlock*, BasicBlock*> identifies a set of edges between two blocks. As it happens, this form is more convenient for the callers, all of whom are deciding whether to break critical edges (except one using it in an assert, but it also has a TI+SuccNum handy).</div>
<div><br></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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div>
<div>+<br></div><div>+/// Determine whether there is a path from From to To within a single function.</div><div>+/// Returns false only if we can prove that once 'From' has been executed then</div><div>
+/// 'To' can not be executed. Conservatively returns true.</div><div>+bool hasPath(const Instruction *From, const Instruction *To,</div><div>+ DominatorTree *DT = 0, LoopInfo *LI = 0);</div></div><div>
<br></div><div>The comment and the name 'hasPath' don't seem to match? I read the comment as meaning 'mayHavePath' or 'isPotentiallyReachable' or some such. Maybe you're trying to say that the definition of 'path' is exactly what we will be able to statically prove? This isn't entirely clear to me.</div>
</div></div></div></blockquote><div><br></div><div>Thanks, 'isPotentiallyReachable' is a much better fit. "If we're at instruction From, could we reach instruction To?" with a conservative 'true' answer.</div>
<div><br></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"><div dir="ltr"><div class="gmail_extra">
<div class="gmail_quote">
<div>+namespace {<br></div><div><div>+</div><div>+// This fixture assists in running the hasPath utility four ways and ensuring</div><div>+// it's correct all ways</div><div>+class HasPathTest : public testing::Test {</div>
<div>+protected:</div><div>+ void ParseAssembly(StringRef Assembly) {</div><div>+ M.reset(new Module("Module", getGlobalContext()));</div><div>+</div><div>+ SMDiagnostic Error;</div><div>+ bool Parsed = ParseAssemblyString(Assembly.str().c_str(), M.get(), Error,</div>
<div>+ M->getContext()) == M.get();</div><div>+ std::string errMsg;</div><div>+ raw_string_ostream os(errMsg);</div><div>+ Error.print("", os);</div><div>+ // Assert, because a bug here means that the test itself is buggy.</div>
<div>+ ASSERT_TRUE(Parsed) << os.str();</div><div>+</div><div>+ Function *F = M->getFunction("test");</div><div>+ ASSERT_TRUE(F != NULL) << "Test must have a function named @test: " << M;</div>
<div>+</div><div>+ A = B = NULL;</div><div>+ for (inst_iterator I = inst_begin(F), E = inst_end(F); I != E; ++I) {</div><div>+ if (I->hasName()) {</div><div>+ if (I->getName() == "A")</div>
<div>+ A = &*I;</div><div>+ else if (I->getName() == "B")</div><div>+ B = &*I;</div><div>+ }</div><div>+ }</div><div>+ ASSERT_TRUE(A != NULL) << "@test must have an instruction %A: " << *F;</div>
<div>+ ASSERT_TRUE(B != NULL) << "@test must have an instruction %B: " << *F;</div><div>+ }</div><div>+</div><div>+ void ExpectPath(bool ExpectedResult) {</div><div>+ static char ID;</div><div>
+ class HasPathTestPass : public FunctionPass {</div><div>+ public:</div><div>+ HasPathTestPass(bool ExpectedResult, Instruction *A, Instruction *B)</div><div>+ : FunctionPass(ID), ExpectedResult(ExpectedResult), A(A), B(B) {}</div>
<div>+</div><div>+ void getAnalysisUsage(AnalysisUsage &AU) const {</div><div>+ AU.setPreservesAll();</div><div>+ AU.addRequired<LoopInfo>();</div><div>+ AU.addRequired<DominatorTree>();</div>
<div>+ }</div><div>+</div><div>+ bool runOnFunction(Function &F) {</div><div>+ if (!F.hasName() || F.getName() != "test")</div><div>+ return false;</div><div>+</div><div>+ LoopInfo *LI = &getAnalysis<LoopInfo>();</div>
<div>+ DominatorTree *DT = &getAnalysis<DominatorTree>();</div><div>+ EXPECT_EQ(hasPath(A, B, 0, 0), ExpectedResult);</div><div>+ EXPECT_EQ(hasPath(A, B, DT, 0), ExpectedResult);</div><div>
+ EXPECT_EQ(hasPath(A, B, 0, LI), ExpectedResult);</div>
<div>+ EXPECT_EQ(hasPath(A, B, DT, LI), ExpectedResult);</div><div>+ return false;</div><div>+ }</div><div>+ bool ExpectedResult;</div><div>+ Instruction *A, *B;</div><div>+ };</div><div>+</div>
<div>+ HasPathTestPass *P = new HasPathTestPass(ExpectedResult, A, B);</div><div>+ PassManager PM;</div><div>+ PM.add(P);</div><div>+ PM.run(*M);</div><div>+ }</div><div>+private:</div><div>+ OwningPtr<Module> M;</div>
<div>+ Instruction *A, *B;</div><div>+};</div><div>+</div><div>+}</div><div>+</div><div>+TEST_F(HasPathTest, SameBlockNoPath) {</div><div>+ ParseAssembly(</div><div>+ "define void @test() {\n"</div><div>
+ "entry:\n"</div>
<div>+ " bitcast i8 undef to i8\n"</div><div>+ " %B = bitcast i8 undef to i8\n"</div><div>+ " bitcast i8 undef to i8\n"</div><div>+ " bitcast i8 undef to i8\n"</div>
<div>+ " %A = bitcast i8 undef to i8\n"</div><div>+ " ret void\n"</div><div>+ "}\n");</div><div>+ ExpectPath(false);</div><div>+}</div></div><div><br></div><div>I'm not a huge fan of this structure of the testing machinery.</div>
<div><br></div><div>I would probably do it slightly differently:</div><div><br></div><div>Have a method in the fixture which builds a dummy pass and passmanager that does nothing other than take a module member and extract the DomTree and other analysis passes into other fixture members. Then you could build up the IR in the module using an IRBuilder (and/or some variant of ParseAssembly, although ParseAssembly should almost certainly be hoisted into a generic IR unit testing helper library), run the pass manager to extract analysis passes, and then have each test directly state its EXPECT_...(...) lines against hasPath.</div>
<div><br></div><div>Does that make sense?</div></div></div></div>
</blockquote></div><br></div><div class="gmail_extra">Sadly, it isn't safe in general to extract a Pass and use it outside the PassManager. For example, it's permitted to free internal data structures upon doFinalization. I really do want the EXPECT_... calls in each test function, and I couldn't figure out how to do it.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">ParseAssembly absolutely deserves to be in a testing library, as it is I'm replicating logic from unittests/ExecutionEngine/JIT/{Multi,}JITTest.cpp and IR/DominatorTreeTest.cpp. Of course, we don't have one. There are already three users of essentially the same functionality, let me see if I can do something about that now.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra" style>Do you have any comments about the implementation of hasPath itself? It's reasonably complex.</div><div class="gmail_extra"><br></div><div class="gmail_extra">
Nick</div></div>