[PATCH] D63604: [Attributor] Deduce "nonnull" attribute

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert accepted this revision.
jdoerfert added a comment.
This revision is now accepted and ready to land.

Small nits that should be fixed but other than that this looks good to me.



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1147
+      return NonNullAA->isAssumedNonNull();
+    }
+    return false;
----------------
I think we should switch the order here. If we have an `AANonNull` for the call site we should use it for sure. Otherwise, look at the paramAttr and ask `isKnownNonZero`. The latter is not necessarily cheap so we should use information computed already first.

You want `CallBase` not `CallInst`, or better, compare the anchoredvalue with the `CS` inst.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1160
+  //       For that, we need to change propagation direction from
+  //       FORWARD to BACKWARD.
+
----------------
The whole "direction" concept is something I talked about but I don't think we actually need it. Let's just say we need to do it to avoid circular conclusions.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1167
+    return ChangeStatus::UNCHANGED;
+  }
+
----------------
This seems like something we can move in the initialize method as it should not change over time.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1171
+
+  if (!NonNullAA || !NonNullAA->isValidState() ||
+      !NonNullAA->isAssumedNonNull()) {
----------------
I improved `getAAFor` such that whenever it returns a non-null pointer the state of that abstract attribute is valid. That means we do not need to check for a valid state at the call sites.


================
Comment at: llvm/test/Transforms/FunctionAttrs/nonnull.ll:4
+; RUN: opt -attributor --attributor-disable=false -S < %s | FileCheck %s --check-prefixes=BOTH,ATTRIBUTOR
 
 declare nonnull i8* @ret_nonnull()
----------------
We need a target layout here. The test is now dependent on the sizes of pointer (I think) and without target layout it might take the host values which would be bad for 32bit machines.


================
Comment at: llvm/test/Transforms/FunctionAttrs/nonnull.ll:170
+; Return value is trivial but currently missing nonnull because 
+; isKnownNonZero(inttoptr(4)) returns false now.
+
----------------
Will be fixed with D64708.


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

https://reviews.llvm.org/D63604