[PATCH] D35741: Add MemorySSA alternative to AliasSetTracker in LICM.

[Daniel Berlin via llvm-commits]

FWIW I suspect MemorySSAAliasSets will be too slow for use by anything for
real.
But i guess we'll see :)


On Tue, Sep 26, 2017 at 5:42 PM, Sanjoy Das via Phabricator <
reviews at reviews.llvm.org> wrote:

> sanjoy requested changes to this revision.
> sanjoy added a comment.
> This revision now requires changes to proceed.
>
> I have added some generic style comments.
>
> As discussed in person, I think we should split out the
> `MemorySSAAliasSets` class, and add some targeted unit tests.
>
> Please also add a comment describing the algorithm at a high level.
>
>
>
> ================
> Comment at: include/llvm/Analysis/MemorySSAAliasSets.h:10
> +// This a a helper class that creates alias sets using MemorySSA, each
> +// set being marked with properties that enablei/disable promotion.
> +// It is used for promotion in the Loop Invariant Code Motion Pass.
> ----------------
> s/enablei/enable/
>
>
> ================
> Comment at: include/llvm/Analysis/MemorySSAAliasSets.h:49
> +    // witout a MemoryLocation.
> +    bool CanNeverPromote=false;
> +    bool LoopInvariantAccess=false;
> ----------------
> The whitespace is a bit off here (clang-format?).
>
>
> ================
> Comment at: include/llvm/Analysis/MemorySSAAliasSets.h:98
> +
> +  void addToAliasSets(const MemoryUse *Use, SmallPtrSetImpl<const
> MemoryAccess*> &Processed) {
> +    Processed.insert(Use);
> ----------------
> You probably should split this out into header and a .cpp -- that way
> telling the interface apart from the implementation will become easier.
>
>
> ================
> Comment at: include/llvm/Support/Debug.h:102
> +///
> +extern bool UseLICMwMSSA;
> +
> ----------------
> I'd prefer to spell this out as UseLICMWithMSSA.
>
>
> ================
> Comment at: include/llvm/Transforms/Scalar/LoopPassManager.h:350
>      PA.preserve<ScalarEvolutionAnalysis>();
> +    if (EnableMSSALoopDep) {
> +      PA.preserve<MemorySSAAnalysis>();
> ----------------
> LLVM style is to avoid braces around single line statements like this.
>
>
> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSAAliasSets.h:10
> +// This a a helper class that creates alias sets using MemorySSA, each
> +// set being marked with properties that enablei/disable promotion.
> +// It is used for promotion in the Loop Invariant Code Motion Pass.
> ----------------
> s/enablei/enable/
>
>
> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSAAliasSets.h:28
> +
> +using namespace llvm;
> +
> ----------------
> Opening namespaces in headers is against LLVM style.
>
>
> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSAAliasSets.h:34
> +  MemorySSA *MSSA;
> +  MemorySSAUpdater *MSSAUpdater;
> +
> ----------------
> I'd prefer having these under accessors.
>
>
> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSAAliasSets.h:43
> +
> +  class SingleMustAliasVec {
> +    MemoryLocation Loc;
> ----------------
> I'm not sure about the name -- how about `MustAliasSet`?  The "single"
> part is implied by the fact that the name is singular, and this is
> semantically a "set" and not a "vec".
>
>
> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSAAliasSets.h:54
> +    bool HasAGoodStore = false;
> +    SingleMustAliasVec(MemoryLocation loc) : Loc(loc),
> CanNeverPromote(false) {
> +      Vec.insert(const_cast<Value *>(Loc.Ptr));
> ----------------
> You probably don't need the `CanNeverPromote(false)` bit.
>
>
> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSAAliasSets.h:58
> +    void addMemoryLocation(Optional<MemoryLocation> MemLoc) {
> +      if (MemLoc == None)
> +        return;
> ----------------
> This will be a line shorter without an early return.
>
>
> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSAAliasSets.h:86
> +  unsigned MemorySSATraversalPenalty = 0;
> +  unsigned CAP_NO_ALIAS_SETS = 30;
> +  unsigned CAP_MSSA_TRAVERSALS = 30;
> ----------------
> These need to be `cl::opt` s
>
> You should also split this header into a .cpp file (and put the cl::opt s
> in  that .cpp file).
>
>
> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSAAliasSets.h:90
> +  void dump() {
> +    for (SingleMustAliasVec *Set : AllSets)
> +      Set->dump();
> ----------------
> You should use `LLVM_ENABLE_DUMP` and `LLVM_DUMP_METHOD` for this.  See
> the definition and declaration of e.g. `DominanceFrontierWrapperPass:
> :dump()`
>
>
> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSAAliasSets.h:96
> +    if(!L)
> +      return true;
> +    return L->isLoopInvariant(V);
> ----------------
> Can be a ternary.  Same for loopContains
>
>
> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSAAliasSets.h:112
> +                    MemoryLocation &MemLocJ) {
> +    if (MemLocI == None) {
> +      ModRefInfo ModRef = AA->getModRefInfo(InstrI, MemLocJ);
> ----------------
> I'd structure this as:
>
> ```
> if (MemLocI)
>   return AA->alias(MemLocI.getValue(), MemLocJ);
> // Rest of the code
> ```
>
>
>
> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSAAliasSets.h:247
> +    // before their children in the worklist and process the worklist in
> order.
> +    SmallVector<DomTreeNode *, 16> Worklist = collectChildrenInLoop(N, L);
> +
> ----------------
> In `loopContains` you've handled the case where `L` is `nullptr`, but
> you've not done the same here.
>
>
> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSAAliasSets.h:253
> +      if (Accesses)
> +        for (auto &Acc : make_range(Accesses->begin(), Accesses->end())) {
> +          if (AllSets.size() > CAP_NO_ALIAS_SETS ||
> ----------------
> Can this be `for (auto &Acc : *Accesses)`?
>
>
> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSAAliasSets.h:260
> +          // Process uses
> +          const MemoryUse *Use = dyn_cast_or_null<MemoryUse>(&Acc);
> +          if (Use)
> ----------------
> You can do:
>
> ```
> if (const auto *Use = dyn_cast_or_null<MemoryUse>(&Acc))
>   addToAliasSets(Use);
> ```
>
> also, the second `if` can be an `else if`.
>
>
> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSAAliasSets.h:284
> +
> +  void replace(Value *LoadI, Value *Replacement) {
> +    for (auto OneAliasSet : AllSets) {
> ----------------
> Can you please give this a more obvious name, like `replaceMemoryLocation`?
>
> Also, the name `LoadI` is misleading -- it isn't the load instruction, but
> it is the memory location, right?
>
>
> https://reviews.llvm.org/D35741
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170926/9a2ab6a4/attachment.html>