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