reames accepted this revision.
reames added a comment.
This revision is now accepted and ready to land.
LGTM, but with two specific required follow ups. If you're not comfortable
committing to both, please don't land this one.
================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:93
+static cl::opt<unsigned> MaxInstCheckedForThrow(
+ "max-inst-checked-for-throw-during-inlining", cl::Hidden,
+ cl::desc("the maximum number of instructions analyzed for may throw during
"
----------------
I'd suggest a name change here. Maybe: "inliner-attribute-window"?
================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1159
+
+ auto MayContainThrowingOrExitingCall = [&](Instruction *RVal,
+ Instruction *RInst) {
----------------
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.
================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1175
+ continue;
+ // Sanity check that the cloned return instruction exists and is a return
+ // instruction itself.
----------------
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.
================
Comment at: llvm/test/Transforms/Inline/ret_attr_update.ll:112
+ ret i8* %s
+}
----------------
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.
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
