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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert marked 2 inline comments as done.
jdoerfert added a comment.

Thanks for looking at this. I'll update the patch asap

In D59919#1535643 <https://reviews.llvm.org/D59919#1535643>, @nicholas wrote:

> CHANGED: build-libcalls               NumNoUnwind                             4526 ->       3382 (   -25.276%)
>
> Why did the number of nounwinds drop?


I haven't looked into this but my initial guess would be that we removed code due to the "returned" information for which we then did not add nounwind. To be honest, I don't see how else it should have happened.



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:127
+        if (Arg.hasReturnedAttr())
+          return gernericValueTraversal(CS.getArgOperand(Arg.getArgNo()), State,
+                                        FollowValueCB, VisitValueCB);
----------------
nicholas wrote:
> 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.
Though there is not really stack space needed I see your point. I'll rewrite the recursion.



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:133
+  // recursion keep a record of the values we followed!
+  if (!FollowValueCB(V, State))
+    return;
----------------
nicholas wrote:
> 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).
The test cases above passes just fine but again I see your point. I will add that one and the one below which breaks as you predicted. I'll rewrite the whole traversal.

```
declare i32 @test2(i32 returned %A);
define i32 @test(i32 %A) {
entry:
  ret i32 %A
unreachableblock:
  %B = call i32 @test2(i32 %B)
  ret i32 %B
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59919