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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
