[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:1159
+
+  auto MayContainThrowingOrExitingCall = [&](Instruction *RVal,
+                                             Instruction *RInst) {
----------------
reames wrote:
> Pull this out as a static helper instead of a lambda, add an assert internally that the two instructions are in the same block.  
> 
> Why?  Because I'm 80% sure the state capture on the lambda isn't needed, and having it as a separate function forces that discipline.  
agreed. I'll do that in this change itself before landing. I am using this static helper in followon change D76792.


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1175
+      continue;
+    // Sanity check that the cloned return instruction exists and is a return
+    // instruction itself.
----------------
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. 


================
Comment at: llvm/test/Transforms/Inline/ret_attr_update.ll:112
+  ret i8* %s
+}
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76140