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

[Anna Thomas via Phabricator via cfe-commits]

anna marked 3 inline comments 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:
> 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 :)




================
Comment at: llvm/test/Transforms/Inline/ret_attr_update.ll:112
+  ret i8* %s
+}
----------------
anna wrote:
> reames wrote:
> > There's a critical missing test case here:
> > - Callee and caller have the same attributes w/different values (i.e. deref)
> > 
> > And thinking through the code, I think there might be a bug here.  It's not a serious one, but the if the callee specifies a larger deref than the caller, it looks like the the smaller value is being written over the larger.
> > 
> > Actually, digging through the attribute code, I think I'm wrong about the bug.  However, you should definitely write the test to confirm and document merging behaviour!
> > 
> > If it does turn out I'm correct, I'm fine with this being addressed in a follow up patch provided that the test is added in this one and isn't a functional issue.  
> will check this.
added test case and documented merge behaviour. No bug in code, since we use the already existing value on attribute.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76140