[PATCH] D151943: [InstCombine] Propagate some func/arg/ret attributes from caller to callsite

Sun Jun 4 11:43:41 PDT 2023

nikic added a comment.

In D151943#4394139 <https://reviews.llvm.org/D151943#4394139>, @goldstein.w.n wrote:

> At the moment FunctionAttrs doesn't do any callsite modifications, only the functions themselves. If you think it fits, however, I'd be okay extending FunctionAttrs. Same for nocapture patch? Also should I wait on D152081 <https://reviews.llvm.org/D152081> or since thats too the attributor still makes sense to try and get something into FunctionAttrs?

I think it's pretty unlikely that the Attributor will be enabled by default anytime soon, so I wouldn't wait on that. I expect a substantial investment in improving Attributor compile-times will be needed to make this even remotely viable.

In D151943#4394179 <https://reviews.llvm.org/D151943#4394179>, @goldstein.w.n wrote:

> Also I think then FunctionAttrs need to be moved to run before inlining:
>
>   ./bin/opt -O3 -print-pipeline-passes
>   ...cgscc(devirt<4>(inline<only-mandatory>,inline,function-attrs<skip-non-recursive>...

Unless you care about non-trivial SCCs, it does run before inlining due to CGSCC interleaving.

An alternative to FunctionAttrs is doing this directly in InlineFunction -- in fact, we already transfer return attributes there in https://github.com/llvm/llvm-project/blob/36f351098cd50809658493d9b2e22a795874bab0/llvm/lib/Transforms/Utils/InlineFunction.cpp#L1366. This could be extended to other attribute kinds. The benefit of doing it during inlining is that it's possible to transfer attributes from the call-site, rather than just the definition, though I'm not sure this is relevant for the cases you're interested in. You want this for the case where there are frontend-generated attributes on the definition, right?

================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:3725
+      // callsite's return value if it is used as the return value of the
+      // caller.
+      std::array CallerReturnAttrPropagations = {Attribute::NoUndef,
----------------
goldstein.w.n wrote:
> nikic wrote:
> > This is incorrect for UB-implying attributes like noundef, if the call is not guaranteed to transfer to the return.
> We check that the return of the callsite is the return value of the function. Are you are worried about the return value escaping then an exception or interrupt between the callsite and return?
Yes.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151943