[PATCH] D21262: [CFLAA] Tag actual arguments as AttrEscaped instead of AttrUnknown
George Burgess IV via llvm-commits
llvm-commits at lists.llvm.org
Mon Jun 13 13:16:23 PDT 2016
george.burgess.iv added a comment.
Yay precision!
> I should have separate the refactoring and the functionality changes into two different patches
So... why wasn't this done, then?
================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:132
@@ +131,3 @@
+class CFLGraph {
+private:
+ typedef Value *Node;
----------------
Redundant `private`
================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:158
@@ -153,3 +157,3 @@
- Edge(Value *From, Value *To, EdgeType W, StratifiedAttrs A)
- : From(From), To(To), Weight(W), AdditionalAttrs(A) {}
+ const EdgeList *getNode(Node N) const {
+ auto Itr = NodeImpls.find(N);
----------------
Looks like all the users of this fail if we return `null`. Is there a reason we try to handle it gracefully here?
================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:164
@@ +163,3 @@
+ }
+ EdgeList *getNode(Node N) {
+ auto Itr = NodeImpls.find(N);
----------------
Same here.
================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:180
@@ +179,3 @@
+
+ CFLGraph() = default;
+ CFLGraph(CFLGraph &&) = default;
----------------
FWIW, I had to remove these ctors from D21233, because they made the windows buildbots angry. Apparently it's a bug in MSVC2013.
If these don't have any use, please remove them from this patch, as well.
================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:237
@@ +236,3 @@
+ Externals.insert(Val);
+ else if (auto CExpr = dyn_cast<ConstantExpr>(Val)) {
+ if (hasUsefulEdges(CExpr))
----------------
Style nit: Braces.
================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:371
@@ -282,1 +370,3 @@
+ struct Edge {
+ // Which value the edge is coming from
----------------
Comment disappeared?
================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:372
@@ +371,3 @@
+ struct Edge {
+ // Which value the edge is coming from
+ Value *From;
----------------
Is there a reason we went from `///` to `//` for these?
================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:483
@@ +482,3 @@
+ // Commit all edges in Output to CFLGraph
+ for (auto const &Edge : Output)
+ addEdge(Edge.From, Edge.To, Edge.Weight, Edge.AdditionalAttrs);
----------------
`const auto &`
================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:502
@@ -383,4 +501,3 @@
// etc.
- if (isMallocLikeFn(&Inst, &TLI) || isCallocLikeFn(&Inst, &TLI)) {
- Output.push_back(Edge(&Inst, &Inst, EdgeType::Assign, AttrNone));
+ if (isMallocLikeFn(Inst, &TLI) || isCallocLikeFn(Inst, &TLI)) {
return;
----------------
Useless braces
================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:504
@@ -386,6 +503,3 @@
return;
- } else if (isFreeCall(&Inst, &TLI)) {
- assert(Inst.getNumArgOperands() == 1);
- auto argVal = Inst.arg_begin()->get();
- Output.push_back(Edge(argVal, argVal, EdgeType::Assign, AttrNone));
+ } else if (isFreeCall(Inst, &TLI)) {
return;
----------------
Please fold this into the above condition
================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:511
@@ -396,3 +510,3 @@
SmallVector<Function *, 4> Targets;
- if (getPossibleTargets(&Inst, Targets)) {
- if (tryInterproceduralAnalysis(Targets, &Inst, Inst.arg_operands()))
+ if (getPossibleTargets(CS, Targets)) {
+ if (tryInterproceduralAnalysis(Targets, Inst, CS.args()))
----------------
Useless braces
================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:520
@@ +519,3 @@
+ // anything, too (unless the result is marked noalias).
+ if (Targets.size() > 1 || (!CS.onlyReadsMemory())) {
+ for (Value *V : CS.args()) {
----------------
Please remove the braces here and below...
(And the parens on `!CS.onlyReadsMemory()`)
================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:520
@@ +519,3 @@
+ // anything, too (unless the result is marked noalias).
+ if (Targets.size() > 1 || (!CS.onlyReadsMemory())) {
+ for (Value *V : CS.args()) {
----------------
george.burgess.iv wrote:
> Please remove the braces here and below...
>
> (And the parens on `!CS.onlyReadsMemory()`)
Why do we care about the size of `Targets` here? Looks like we'll mark nothing as escaped if we can't find a target of a function.
================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:526
@@ +525,3 @@
+ if (!Inst->getType()->isVoidTy()) {
+ auto Fn = CS.getCalledFunction();
+ if (Fn == nullptr || !Fn->doesNotAlias(0))
----------------
`auto *`
================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:527
@@ +526,3 @@
+ auto Fn = CS.getCalledFunction();
+ if (Fn == nullptr || !Fn->doesNotAlias(0))
+ addEdge(Inst, Inst, EdgeType::Assign, AttrUnknown);
----------------
Looks like we won't add this to the sets if `Fn->doesNotAlias(0)`.
================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:660
@@ -735,1 +659,3 @@
}
+ SmallPtrSet<Value *, 8> getExternalValues() {
+ return std::move(ExternalValues);
----------------
Is there a reason we can't hand back a const& here instead?
Also, same thoughts about things that look+feel like accessors (but call move) as in D21261.
================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:663
@@ +662,3 @@
+ }
+ SmallPtrSet<Value *, 8> getEscapedValues() {
+ return std::move(EscapedValues);
----------------
And here
================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:824
@@ -905,3 +823,3 @@
// Special handling for globals and arguments
- auto ProcessGlobalOrArgValue = [&SetBuilder](Value &Val) {
+ auto ProcessExternalValue = [&SetBuilder](Value &Val) {
SetBuilder.add(&Val);
----------------
This seems pointless to have as a lambda if we're just using it in one place.
================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:834
@@ +833,3 @@
+ // Special handling for escaped values
+ auto ProcessEscapedValue = [&SetBuilder](Value &Val) {
+ SetBuilder.add(&Val);
----------------
Same for this.
================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:840
@@ +839,3 @@
+
+ auto Externals = GraphBuilder.getExternalValues();
+ for (auto External : Externals)
----------------
If we hand back a const& above, this needs to become a `const auto&`. Also, why not just call `GraphBuilder.getExternalValues()` in the range for?
================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:841
@@ +840,3 @@
+ auto Externals = GraphBuilder.getExternalValues();
+ for (auto External : Externals)
+ ProcessExternalValue(*External);
----------------
`auto *`
================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:844
@@ +843,3 @@
+
+ auto Escapes = GraphBuilder.getEscapedValues();
+ for (auto Escape : Escapes)
----------------
This, too.
================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:845
@@ +844,3 @@
+ auto Escapes = GraphBuilder.getEscapedValues();
+ for (auto Escape : Escapes)
+ ProcessEscapedValue(*Escape);
----------------
`auto *`
================
Comment at: test/Analysis/CFLAliasAnalysis/attr-escape.ll:53
@@ +52,3 @@
+; CHECK: NoAlias: i32* %c, i32** %a
+define void @test_external_call_readonly(i32* %x) {
+ %a = alloca i32*, align 8
----------------
Can we have a test for callsites marked as `readonly`, (if we currently support them) as well?
http://reviews.llvm.org/D21262
More information about the llvm-commits
mailing list