[PATCH] D59919: [Attributor] Deduce "returned" argument attribute

[Nick Lewycky via Phabricator via llvm-commits]

nicholas added a comment.

> CHANGED: build-libcalls               NumNoUnwind                             4526 ->       3382 (   -25.276%)

Why did the number of nounwinds drop?



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:115
+template <typename StateTy>
+static void gernericValueTraversal(Value *V, StateTy &State,
+                                   followValueCB_t<StateTy> &FollowValueCB,
----------------
Typo in "gerneric", should be "generic".


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:127
+        if (Arg.hasReturnedAttr())
+          return gernericValueTraversal(CS.getArgOperand(Arg.getArgNo()), State,
+                                        FollowValueCB, VisitValueCB);
----------------
LLVM generally has a preference for not recursing like this, it means that the amount of stack space we need depends on the input IR and it's hard for a user of llvm as a library to foresee or handle an out of stack condition.

Common practice is to structure it as a loop like:
```
SmallVector<Value *, 32> Worklist;
SmallSet<Value *> Visited;
Worklist.push_back(V);
do {
  Value *V = Worklist.pop_back_val();
  if (!Visited.insert(V).second)
    continue;
  V = V->stripPointerCasts();
  // ...
} while (!Worklist.empty());
```

Also, consider having some sort of loop iteration limit as a safety value against runaway compile time.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:133
+  // recursion keep a record of the values we followed!
+  if (!FollowValueCB(V, State))
+    return;
----------------
Offhand, I think placing this after the CS check is incorrect. I haven't tried it out, but I expect the testcase that triggers infinite loop to look something like this:

```
define i32 @test(i32 %A) {
entry:
  ret i32 0
unreachableblock:
  %B = call i32 @test(i32 %B)
  ret i32 %B
}
```

which should pass the verifier and trigger an infinite loop if you call gernericValueTraversal on %B.

Also, if you really need a callback and not just a SmallSet named Visited, I'd suggest calling the callback immediately before adding each value to the Worklist (or as written not, call it on each value before recursing).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59919/new/

https://reviews.llvm.org/D59919