<div dir="ltr">On Mon, Jun 10, 2013 at 5:24 PM, Nick Lewycky <span dir="ltr"><<a href="mailto:nlewycky@google.com" target="_blank" class="cremed">nlewycky@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">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" class="cremed">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>Mind putting the next version into Phab? Much easier to review there.</div><div><br></div><div><div>+/// FindFunctionBackedges - Analyze the specified function to find all of the</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><br></div><div>+///</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><br></div><div>+</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><br></div><div>+</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><br></div><div>+</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><br></div><div><div>+namespace {</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>