<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 20 August 2014 18:08, Nick Lewycky <span dir="ltr"><<a href="mailto:nlewycky@google.com" target="_blank">nlewycky@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I think this is worth landing and any bug fixes can be incremental development?<br></blockquote><div><br></div><div>...but wait for Hal?</div><div><br></div><div>Nick</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
================<br>
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:170<br>
@@ +169,3 @@<br>
+ DenseMap<Function *, Optional<FunctionInfo>> Cache;<br>
+ std::forward_list<FunctionHandle> handles;<br>
+<br>
----------------<br>
handles -> Handles, I think><br>
<br>
================<br>
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:241-242<br>
@@ +240,4 @@<br>
<div class="">+// Nothing - Unknown instruction. Program should die.<br>
</div>+// Just(false) - Known instruction, but not supported (probably disabled<br>
+// while trying to find a bug). Discard results from GetEdgesVisitor.<br>
+// Just(true) - Known instruction, results are valid. Continue as normal.<br>
----------------<br>
"disabled while trying to find a bug"? An example that explains when this comes up in production use would be better. Unless it really is a debugging feature?<br>
<br>
================<br>
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:280<br>
@@ +279,3 @@<br>
+ void visitPHINode(PHINode &Inst) {<br>
+ for (unsigned I = 0, E = Inst.getNumIncomingValues(); I < E; ++I) {<br>
+ Value *Val = Inst.getIncomingValue(I);<br>
----------------<br>
"I < E" is usually "I != E" in llvm, though that causes no particular benefit here.<br>
<br>
================<br>
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:286-289<br>
@@ +285,6 @@<br>
+<br>
+ void visitGetElementPtrInst(GetElementPtrInst &Inst) {<br>
+ Value *Op = Inst.getPointerOperand();<br>
+ Output.push_back({&Inst, Op, EdgeWeight::Assign, AttrNone});<br>
+ }<br>
+<br>
----------------<br>
What about the index arguments? Suppose you have<br>
inty = ptrtoint ptry<br>
GEP (ptrx-ptry), inty<br>
?<br>
<br>
<br>
================<br>
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:291-296<br>
@@ +290,8 @@<br>
+<br>
+ void visitSelectInst(SelectInst &Inst) {<br>
+ auto *TrueVal = Inst.getTrueValue();<br>
+ auto *FalseVal = Inst.getFalseValue();<br>
+ Output.push_back({&Inst, TrueVal, EdgeWeight::Assign, AttrNone});<br>
+ Output.push_back({&Inst, FalseVal, EdgeWeight::Assign, AttrNone});<br>
+ }<br>
+<br>
----------------<br>
Only the true and false values but not the condition, eh. You wanna hear something horrible?<br>
<br>
void foo(char *p) {<br>
uintptr_t x = 0;<br>
uintptr_t y = (uintptr_t)p;<br>
for (int i = 0; i < 8*sizeof(char*); ++i, x <<= 1) {<br>
int bit = (y>>(8*sizeof(char*)-1)) & 1;<br>
x += bit ? 1 : 0; // SelectInst here.<br>
}<br>
char *q = (char*)x;<br>
// don't answer noalias(p, q).<br>
}<br>
<br>
<br>
================<br>
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:312-314<br>
@@ +311,5 @@<br>
+<br>
+ static bool isFunctionExternal(Function *Fn) {<br>
+ return !Fn->isDeclaration() && Fn->hasLocalLinkage();<br>
+ }<br>
+<br>
----------------<br>
Eh? I would've expected it's external if declaration || !hasLocalLinkage. Is this function the negative of its name?<br>
<br>
================<br>
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:386<br>
@@ +385,3 @@<br>
+ StratifiedAttrs Externals;<br>
+ for (unsigned X = 0; X < RetVals.size(); ++X) {<br>
+ auto MaybeInfo = Sets.find(RetVals[X]);<br>
----------------<br>
Does RetVals change size inside this loop? If not, the "for (unsigned X = 0, XE = RetVals.size(); X != XE; ++X) {" formulation makes that clear.<br>
<br>
(Oh and again at line 381 for Parameters, 415 for Arguments.)<br>
<br>
================<br>
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:406-407<br>
@@ +405,4 @@<br>
+<br>
+ if (Parameters.size() != Arguments.size())<br>
+ return false;<br>
+<br>
----------------<br>
You could also check Fn->isVarArgs() way up in the early exit detection code instead? If it's not varargs then #params = #args? Unless the call is indirect (ie., through a bitcast).<br>
<br>
================<br>
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:414<br>
@@ +413,3 @@<br>
+ // Arguments[I] and Arguments[X], but our algorithm will produce<br>
+ // extermely similar, and equally correct, results either way)<br>
+ for (unsigned I = 0; I < Arguments.size(); ++I) {<br>
----------------<br>
Typo, "extermely" -> "extremely".<br>
<br>
================<br>
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:738-739<br>
@@ +737,4 @@<br>
+ while (!Worklist.empty()) {<br>
+ auto Node = Worklist.back();<br>
+ Worklist.pop_back();<br>
+<br>
----------------<br>
auto Node = Worklist.pop_back_val()?<br>
<br>
================<br>
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:793<br>
@@ +792,3 @@<br>
+//===----------------------------------------------------------------------===//<br>
+// Lengthy CFL AA methods<br>
+//===----------------------------------------------------------------------===//<br>
----------------<br>
"Lengthy"?<br>
<br>
<a href="http://reviews.llvm.org/D4551" target="_blank">http://reviews.llvm.org/D4551</a><br>
<br>
<br>
</blockquote></div><br></div></div>