[PATCH] D76140: [InlineFunction] update attributes during inlining

[Anna Thomas via Phabricator via cfe-commits]

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