[PATCH] D84733: [Attributor] Check nonnull attribute violation in AAUndefinedBehavior

[Johannes Doerfert via Phabricator via llvm-commits]

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

LGTM. Some minor nits below. I guess this doesn't affect other tests because of the `noundef` restriction. We should really derive that one as well.



================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:1993
+      unsigned ArgNum =
+          std::min(CB->getNumArgOperands(), (unsigned)Callee->arg_size());
+      for (unsigned idx = 0; idx < ArgNum; idx++) {
----------------
okura wrote:
> jdoerfert wrote:
> > You don't need a callee, just use the call base.
> Do you mean that I don't have to be aware of  `Callee->arg_size()` anywhere?
> In the case of callbacks, `CB.getNumArgOperands() > Callee->arg_size()` holds and I got segfault when `getArg` is called.
> Can I get the callee argument position directly from `CallBase`?
Oh, OK. For any variadic function the call can have more operands than the callee arguments. I guess what you really want are the abstract call sites you can get from the call base. Let's keep it like this for now.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:2007
+          break;
+        Value *ArgVal = CB.getArgOperand(idx);
+        IRPosition CalleeArgumentIRP =
----------------
I guess the easiest is to check if it is a pointer early on. That should avoid running the code below in case it can never trigger.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:2021
+        if (!ValueSimplifyAA.isKnown() || !SimplifiedVal.hasValue())
+          continue;
+        if (!isa<ConstantPointerNull>(*SimplifiedVal.getValue()))
----------------
Not having a value and having the value `undef` are also all violations if you will. The former means it is dead anyway, and `undef` can degenerate to `null` whenever you want it to.


================
Comment at: llvm/test/Transforms/Attributor/undefined_behavior.ll:598
+;
+  store i32 0, i32* %a
+  ret void
----------------
Next up, we can derive `noundef` for this case (amongst others), given that we actually access the pointer for sure. An undef value would cause UB so %a cannot be undef. Basically the same way we should already derive `nonnull` for this case. Later ;)


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

https://reviews.llvm.org/D84733