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

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

In D76140#1922162 <https://reviews.llvm.org/D76140#1922162>, @jdoerfert wrote:

> In addition to @reames correctness concerns this is not valid for a different reason. You cannot backwards propagate information in the general case:
>
>   __attribute__((nonnull)) int *foo(int c) {
>      int *A = unknown();
>      int *B = unknown();
>      do_sth_with_ptr_and_may_use_nonnull(A, B);
>      return c ? A : B; // or if (C) return A; else return B;
>   }
>
>
> Knowing `foo` returns a `nonnull` value cannot be used to annotate `A` or `B`.


Good catch, I'd missed that.

> I think this is generally problematic as it basically special cases a situation which we can handle in general just fine.
>  Let's assume we inline and keep the `nonnull` on the return values around, we run `nonnull` deduction on the caller and we get the `nonnull` where it is correct.
>  The benefit of doing it this way (D75825 <https://reviews.llvm.org/D75825>) is that it will also work with all other attributes as well without reinventing all the logic in the Attributor.

I disagree, mostly in the framing of this as an either/or.  If we can cheaply match IR patterns during inlining and use attributes instead of assumes, we should.  The other option requires IR churn for minimal value since we'll fold the inserted assumes back into attributes in the end anyway.  We clearly should use assumes for the general case, but that doesn't mean we shouldn't specialize the common case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76140