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

[Anna Thomas via Phabricator via llvm-commits]

anna added a comment.

In D76140#1922292 <https://reviews.llvm.org/D76140#1922292>, @reames wrote:

> 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.


Just noticed this. I think we can special case this to: the return and def call are in the same basic block (it will take care of the if-else in the example) and has only one use which is the return. The latter takes care of avoiding incorrect semantics in that call `do_sth_with_ptr_and_may_use_nonnull`  or any other use that depends on knowing which of the ptr is non-null.

> 
> 
>> 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.

The above restrictions (which makes it conservative, but we have cases like that) should allow us cleanly place all the return attributes on the calls within the body, I think.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76140