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

[Sanjoy Das via llvm-commits]

On Tue, Sep 26, 2017 at 5:57 PM, Daniel Berlin <dberlin at dberlin.org> wrote:
> FWIW I suspect MemorySSAAliasSets will be too slow for use by anything for
> real.

I was under the impression is that it should be no worse than
AliasSetTracker, and better in some cases (large blocks with very few
memory operations).  Otherwise I don't see a reason to do this.

-- Sanjoy

> 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
>>
>>
>>
>