[PATCH] D65593: [Attributor][MustExec] Deduce dereferenceable and nonnull attribute by exploring path

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

I only reviewed the MustExecute parts as the Attributor part probably needs a rebase. I think this is needed and I'll be more responsive this time.



================
Comment at: llvm/include/llvm/Analysis/MustExecute.h:395
                                    const Instruction *PP);
+  struct KnownAbstractState {
+    virtual ~KnownAbstractState () = default;
----------------
Newline before and comment describing this struct and its use briefly.


================
Comment at: llvm/include/llvm/Analysis/MustExecute.h:411
+    /// Additive identity.
+    static KnownAbstractState Bottom();
+  };
----------------
I would not call it `Multiplicative`/`Additive` identity, nor `Top`/`Bottom`. I would stick to the Attributor spellings of "best/worst state". The others are overloaded too often (IMHO).


================
Comment at: llvm/include/llvm/Analysis/MustExecute.h:429
+    if (MemoizeResult.count(PPBegin))
+      return *MemoizeResult.lookup(PPBegin);
+
----------------
If you keep pointer values, this is shorter: `if (AState *State = MemoizeResult.lookup(PPBegin)) return *State;`


================
Comment at: llvm/include/llvm/Analysis/MustExecute.h:452
+      }
+    }
+
----------------
I think I would prefer that we first explore range completely and only collect branch instructions. If range was not enough to cause stopExplore to trigger, we start exploring branches. The reason is that range information is "better" than child information as it holds without the need to explore a second path. We should also have a parameter to limit the search.



================
Comment at: llvm/include/llvm/Analysis/MustExecute.h:454
+
+    MemoizeResult[PPBegin] = &AS;
+    return AS;
----------------
Is it wise to store the address of the local object `AS` here? Maybe we need the map to own the state.


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

https://reviews.llvm.org/D65593