[Attributor] Remove dereferenceable_or_null when nonull is present

[uenoku]

Summary

This patch prevents the simultaneous presence of dereferenceable and dereferenceable_or_null attribute

[jdoerfert]

This was a problem for a while now:
Can we have a test case to make sure we do not lose information, e.g. deref_or_null(100) nonnull deref(4) should not remove the deref_or_null(100).

llvm/test/Transforms/FunctionAttrs/dereferenceable.ll

32–33

FIXME can be removed.

[uenoku]

Add test

[uenoku]

In D70789#1762114, @jdoerfert wrote:

This was a problem for a while now:
Can we have a test case to make sure we do not lose information, e.g. deref_or_null(100) nonnull deref(4) should not remove the deref_or_null(100).

In my understanding, deref_or_null(100) and nonnull imply deref(100) so deref_or_null(100) can be deleted safely, correct?

[uenoku]

Remove fixme

[jdoerfert]

LGTM.

In D70789#1762474, @uenoku wrote:

In D70789#1762114, @jdoerfert wrote:

This was a problem for a while now:
Can we have a test case to make sure we do not lose information, e.g. deref_or_null(100) nonnull deref(4) should not remove the deref_or_null(100).

In my understanding, deref_or_null(100) and nonnull imply deref(100) so deref_or_null(100) can be deleted safely, correct?

Yes.

[lebedev.ri]

In D70789#1763283, @jdoerfert wrote:

LGTM.

In D70789#1762474, @uenoku wrote:

In D70789#1762114, @jdoerfert wrote:

This was a problem for a while now:
Can we have a test case to make sure we do not lose information, e.g. deref_or_null(100) nonnull deref(4) should not remove the deref_or_null(100).

More generally, we do already merge deref_or_null(x) deref(y) -> deref(max(x, y)) ?

[uenoku]

In D70789#1763315, @lebedev.ri wrote:

In D70789#1763283, @jdoerfert wrote:

LGTM.

In D70789#1762474, @uenoku wrote:

In D70789#1762114, @jdoerfert wrote:

This was a problem for a while now:
Can we have a test case to make sure we do not lose information, e.g. deref_or_null(100) nonnull deref(4) should not remove the deref_or_null(100).

More generally, we do already merge deref_or_null(x) deref(y) -> deref(max(x, y)) ?

Yes, please see the comment

llvm/lib/Transforms/IPO/Attributor.cpp

2944–2947

The max of deref(x) and deref_or_null(y) is taken here.

[lebedev.ri]

llvm/lib/Transforms/IPO/Attributor.cpp

2944–2947

Cool, thanks, just checking :)

llvm/lib/Transforms/IPO/Attributor.cpp

 }
 
 struct AADereferenceableImpl : AADereferenceable {
   AADereferenceableImpl(const IRPosition &IRP) : AADereferenceable(IRP) {}
   using StateType = DerefState;
 
   void initialize(Attributor &A) override {
     SmallVector<Attribute, 4> Attrs;
     getAttrs({Attribute::Dereferenceable, Attribute::DereferenceableOrNull},
              Attrs);
     for (const Attribute &Attr : Attrs)
       takeKnownDerefBytesMaximum(Attr.getValueAsInt());
 
     NonNullAA = &A.getAAFor<AANonNull>(*this, getIRPosition());
 
     const IRPosition &IRP = this->getIRPosition();
     bool IsFnInterface = IRP.isFnInterfaceKind();
     const Function *FnScope = IRP.getAnchorScope();
     if (IsFnInterface && (!FnScope || !FnScope->hasExactDefinition()))
       indicatePessimisticFixpoint();
bool followUse(Attributor &A, const Use *U, const Instruction *I) {
     bool IsNonNull = false;
     bool TrackUse = false;
     int64_t DerefBytes = getKnownNonNullAndDerefBytesForUse(
         A, *this, getAssociatedValue(), U, I, IsNonNull, TrackUse);
     takeKnownDerefBytesMaximum(DerefBytes);
     return TrackUse;
   }
 
+  /// See AbstractAttribute::manifest(...).
+  ChangeStatus manifest(Attributor &A) override {
+    ChangeStatus Change = AADereferenceable::manifest(A);
+    if (isAssumedNonNull() && hasAttr(Attribute::DereferenceableOrNull)) {
+      removeAttrs({Attribute::DereferenceableOrNull});
+      return ChangeStatus::CHANGED;
+    }
+    return Change;
+  }
+
   void getDeducedAttributes(LLVMContext &Ctx,
                             SmallVectorImpl<Attribute> &Attrs) const override {
     // TODO: Add *_globally support
     if (isAssumedNonNull())
       Attrs.emplace_back(Attribute::getWithDereferenceableBytes(
           Ctx, getAssumedDereferenceableBytes()));
     else
       Attrs.emplace_back(Attribute::getWithDereferenceableOrNullBytes(

llvm/test/Transforms/FunctionAttrs/dereferenceable.ll

 ; TEST 3
 ; GEP inbounds
 define i32* @test3_1(i32* dereferenceable(8) %0) local_unnamed_addr {
 ; ATTRIBUTOR: define nonnull dereferenceable(4) i32* @test3_1(i32* nofree nonnull readnone dereferenceable(8) "no-capture-maybe-returned" %0)
   %ret = getelementptr inbounds i32, i32* %0, i64 1
   ret i32* %ret
 }
 
 define i32* @test3_2(i32* dereferenceable_or_null(32) %0) local_unnamed_addr {
-; FIXME: We should not have both deref(x) and deref_or_null(y) with x >= y.
-; ATTRIBUTOR: define nonnull dereferenceable(16) i32* @test3_2(i32* nofree nonnull readnone dereferenceable(32) dereferenceable_or_null(32) "no-capture-maybe-returned" %0)
+; ATTRIBUTOR: define nonnull dereferenceable(16) i32* @test3_2(i32* nofree nonnull readnone dereferenceable(32) "no-capture-maybe-returned" %0)
   %ret = getelementptr inbounds i32, i32* %0, i64 4
   ret i32* %ret
 }
 
 define i32* @test3_3(i32* dereferenceable(8) %0, i32* dereferenceable(16) %1, i1 %2) local_unnamed_addr {
 ; ATTRIBUTOR: define nonnull dereferenceable(4) i32* @test3_3(i32* nofree nonnull readnone dereferenceable(8) "no-capture-maybe-returned" %0, i32* nofree nonnull readnone dereferenceable(16) "no-capture-maybe-returned" %1, i1 %2) local_unnamed_addr
   %ret1 = getelementptr inbounds i32, i32* %0, i64 1
   %ret2 = getelementptr inbounds i32, i32* %1, i64 2

 
 define i32* @test_for_minus_index(i32* %p) {
 ; FIXME: This should have a return dereferenceable(8) but we need to make sure it will work in loops as well.
 ; ATTRIBUTOR: define nonnull i32* @test_for_minus_index(i32* nofree nonnull writeonly "no-capture-maybe-returned" %p)
   %q = getelementptr inbounds i32, i32* %p, i32 -2
   store i32 1, i32* %q
   ret i32* %q
 }
+
+define void @deref_or_null_and_nonnull(i32* dereferenceable_or_null(100) %0) {
+; ATTRIBUTOR: define void @deref_or_null_and_nonnull(i32* nocapture nofree nonnull writeonly dereferenceable(100) %0)
+  store i32 1, i32* %0
+  ret void
+}