[PATCH] LLVM CFL Alias Analysis -- Algorithm
Nick Lewycky
nlewycky at google.com
Wed Aug 20 18:08:59 PDT 2014
I think this is worth landing and any bug fixes can be incremental development?
================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:170
@@ +169,3 @@
+ DenseMap<Function *, Optional<FunctionInfo>> Cache;
+ std::forward_list<FunctionHandle> handles;
+
----------------
handles -> Handles, I think>
================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:241-242
@@ +240,4 @@
+// Nothing - Unknown instruction. Program should die.
+// Just(false) - Known instruction, but not supported (probably disabled
+// while trying to find a bug). Discard results from GetEdgesVisitor.
+// Just(true) - Known instruction, results are valid. Continue as normal.
----------------
"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?
================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:280
@@ +279,3 @@
+ void visitPHINode(PHINode &Inst) {
+ for (unsigned I = 0, E = Inst.getNumIncomingValues(); I < E; ++I) {
+ Value *Val = Inst.getIncomingValue(I);
----------------
"I < E" is usually "I != E" in llvm, though that causes no particular benefit here.
================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:286-289
@@ +285,6 @@
+
+ void visitGetElementPtrInst(GetElementPtrInst &Inst) {
+ Value *Op = Inst.getPointerOperand();
+ Output.push_back({&Inst, Op, EdgeWeight::Assign, AttrNone});
+ }
+
----------------
What about the index arguments? Suppose you have
inty = ptrtoint ptry
GEP (ptrx-ptry), inty
?
================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:291-296
@@ +290,8 @@
+
+ void visitSelectInst(SelectInst &Inst) {
+ auto *TrueVal = Inst.getTrueValue();
+ auto *FalseVal = Inst.getFalseValue();
+ Output.push_back({&Inst, TrueVal, EdgeWeight::Assign, AttrNone});
+ Output.push_back({&Inst, FalseVal, EdgeWeight::Assign, AttrNone});
+ }
+
----------------
Only the true and false values but not the condition, eh. You wanna hear something horrible?
void foo(char *p) {
uintptr_t x = 0;
uintptr_t y = (uintptr_t)p;
for (int i = 0; i < 8*sizeof(char*); ++i, x <<= 1) {
int bit = (y>>(8*sizeof(char*)-1)) & 1;
x += bit ? 1 : 0; // SelectInst here.
}
char *q = (char*)x;
// don't answer noalias(p, q).
}
================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:312-314
@@ +311,5 @@
+
+ static bool isFunctionExternal(Function *Fn) {
+ return !Fn->isDeclaration() && Fn->hasLocalLinkage();
+ }
+
----------------
Eh? I would've expected it's external if declaration || !hasLocalLinkage. Is this function the negative of its name?
================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:386
@@ +385,3 @@
+ StratifiedAttrs Externals;
+ for (unsigned X = 0; X < RetVals.size(); ++X) {
+ auto MaybeInfo = Sets.find(RetVals[X]);
----------------
Does RetVals change size inside this loop? If not, the "for (unsigned X = 0, XE = RetVals.size(); X != XE; ++X) {" formulation makes that clear.
(Oh and again at line 381 for Parameters, 415 for Arguments.)
================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:406-407
@@ +405,4 @@
+
+ if (Parameters.size() != Arguments.size())
+ return false;
+
----------------
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).
================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:414
@@ +413,3 @@
+ // Arguments[I] and Arguments[X], but our algorithm will produce
+ // extermely similar, and equally correct, results either way)
+ for (unsigned I = 0; I < Arguments.size(); ++I) {
----------------
Typo, "extermely" -> "extremely".
================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:738-739
@@ +737,4 @@
+ while (!Worklist.empty()) {
+ auto Node = Worklist.back();
+ Worklist.pop_back();
+
----------------
auto Node = Worklist.pop_back_val()?
================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:793
@@ +792,3 @@
+//===----------------------------------------------------------------------===//
+// Lengthy CFL AA methods
+//===----------------------------------------------------------------------===//
----------------
"Lengthy"?
http://reviews.llvm.org/D4551
More information about the llvm-commits
mailing list