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

[Philip Reames via Phabricator via cfe-commits]

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