anna marked an inline comment as done. anna added inline comments.
================ Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1175 + continue; + // Sanity check that the cloned return instruction exists and is a return + // instruction itself. ---------------- anna wrote: > anna wrote: > > reames wrote: > > > Ok, after staring at it a bit, I've convinced myself the code here is > > > correct, just needlessly conservative. > > > > > > What you're doing is: > > > If the callees return instruction and returned call both map to the same > > > instructions once inlined, determine whether there's a possible exit > > > between the inlined copy. > > > > > > What you could be doing: > > > If the callee returns a call, check if *in the callee* there's a possible > > > exit between call and return, then apply attribute to cloned call. > > > > > > The key difference is when the caller directly returns the result vs uses > > > it locally. The result here is that your transform is much more narrow > > > in applicability than it first appears. > > yes, thanks for pointing it out. I realized it after our offline discussion > > :) > > For now, I will add a FIXME testcase which showcases the difference in code > > and handle that testcase in a followon change. > > The key difference is when the caller directly returns the result vs uses > > it locally. The result here is that your transform is much more narrow in > > applicability than it first appears. > I tried multiple test cases to showcase the difference between the two ideas > above but failed. Specifically, `simplifyInstruction` used during inlining > the callee is not too great at optimizing the body. For example, see added > testcase `test7`. > > I also tried the less restrictive version (check the safety of the > optimization in the callee itself, and do the attribute update on the cloned > instruction), but didn't see any testcases in clang that needed update. Of > course, that doesn't mean anything :) > > Clarified this with Philip offline. The current patch is not restrictive. In fact, now that I think of it, sometimes, it may be better - `simplifyInstruction` can fold away instructions and reduce the "window size" between the RV and the ReturnInst. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76140/new/ https://reviews.llvm.org/D76140 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
