[PATCH] D65402: [Attributor][MustExec] Deduce dereferenceable and nonnull attribute using MustBeExecutedContextExplorer

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert requested changes to this revision.
jdoerfert added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:727
+      std::function<const State(const Instruction *I)> Pred,
+      const Instruction *PP, State Init) {
+
----------------
Could we call it `StateType`, make the `std::function` a `function_ref`, and `PP` a reference please.

Also, I think it makes sense to have an early exit. Maybe make the predicate a `function_ref<bool(const Instruction &, State &)>` which updates the state in place if necessary and returns true as long as it wants to continue exploring.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:485
+  return Ret;
+}
+
----------------
Very cool, but make it static or put it in an anonymous namespace.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1218
+      Pred, &getAnchorScope().getEntryBlock().front(),
+      getWorst<BooleanState>());
 
----------------
I thought about this some more:
1) Should we do this traversal for nonnull explicitly or just do it for deref and then have nonnull ask the deref attribute if nonnul is implied. (for now most use cases will not have null as a valid pointer)
2) We should not traverse the context of the same instruction multiple times, or at least not in every update. What we could do is run this in the initialize and remember the AA's that would imply the current one (line 1202). In the update we then only check if any of the implied ones is *known* if not we try to determine it the usual way.


(` &getAnchorScope().getEntryBlock().front(),` will become `getCtxI()`)


================
Comment at: llvm/test/Transforms/InferFunctionAttrs/dereferenceable.ll:11
 ; CHECK-LABEL: @PR21780(double* %ptr)
+; ATTRIBUTOR-LABEL: @PR21780(double* nonnull dereferenceable(32) %ptr)
   ; GEP of index 0 is simplified away.
----------------
Could we have more versions of this test:

Positive:
  - only the access to `%arrayidx3` should suffice
  - all accesses but without the inbounds keyword
Negative:
  - only the access to `%arrayidx3` without the inbound keyword


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

https://reviews.llvm.org/D65402