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