<div dir="ltr">FWIW I suspect MemorySSAAliasSets will be too slow for use by anything for real.<div>But i guess we'll see :)<br><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Sep 26, 2017 at 5:42 PM, Sanjoy Das via Phabricator <span dir="ltr"><<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">sanjoy requested changes to this revision.<br>
sanjoy added a comment.<br>
This revision now requires changes to proceed.<br>
<br>
I have added some generic style comments.<br>
<br>
As discussed in person, I think we should split out the `MemorySSAAliasSets` class, and add some targeted unit tests.<br>
<br>
Please also add a comment describing the algorithm at a high level.<br>
<br>
<br>
<br>
================<br>
Comment at: include/llvm/Analysis/<wbr>MemorySSAAliasSets.h:10<br>
+// This a a helper class that creates alias sets using MemorySSA, each<br>
+// set being marked with properties that enablei/disable promotion.<br>
+// It is used for promotion in the Loop Invariant Code Motion Pass.<br>
----------------<br>
s/enablei/enable/<br>
<br>
<br>
================<br>
Comment at: include/llvm/Analysis/<wbr>MemorySSAAliasSets.h:49<br>
+ // witout a MemoryLocation.<br>
+ bool CanNeverPromote=false;<br>
+ bool LoopInvariantAccess=false;<br>
----------------<br>
The whitespace is a bit off here (clang-format?).<br>
<br>
<br>
================<br>
Comment at: include/llvm/Analysis/<wbr>MemorySSAAliasSets.h:98<br>
+<br>
+ void addToAliasSets(const MemoryUse *Use, SmallPtrSetImpl<const MemoryAccess*> &Processed) {<br>
+ Processed.insert(Use);<br>
----------------<br>
You probably should split this out into header and a .cpp -- that way telling the interface apart from the implementation will become easier.<br>
<br>
<br>
================<br>
Comment at: include/llvm/Support/Debug.h:<wbr>102<br>
+///<br>
+extern bool UseLICMwMSSA;<br>
+<br>
----------------<br>
I'd prefer to spell this out as UseLICMWithMSSA.<br>
<br>
<br>
================<br>
Comment at: include/llvm/Transforms/<wbr>Scalar/LoopPassManager.h:350<br>
PA.preserve<<wbr>ScalarEvolutionAnalysis>();<br>
+ if (EnableMSSALoopDep) {<br>
+ PA.preserve<MemorySSAAnalysis><wbr>();<br>
----------------<br>
LLVM style is to avoid braces around single line statements like this.<br>
<br>
<br>
================<br>
Comment at: include/llvm/Transforms/Utils/<wbr>MemorySSAAliasSets.h:10<br>
+// This a a helper class that creates alias sets using MemorySSA, each<br>
+// set being marked with properties that enablei/disable promotion.<br>
+// It is used for promotion in the Loop Invariant Code Motion Pass.<br>
----------------<br>
s/enablei/enable/<br>
<br>
<br>
================<br>
Comment at: include/llvm/Transforms/Utils/<wbr>MemorySSAAliasSets.h:28<br>
+<br>
+using namespace llvm;<br>
+<br>
----------------<br>
Opening namespaces in headers is against LLVM style.<br>
<br>
<br>
================<br>
Comment at: include/llvm/Transforms/Utils/<wbr>MemorySSAAliasSets.h:34<br>
+ MemorySSA *MSSA;<br>
+ MemorySSAUpdater *MSSAUpdater;<br>
+<br>
----------------<br>
I'd prefer having these under accessors.<br>
<br>
<br>
================<br>
Comment at: include/llvm/Transforms/Utils/<wbr>MemorySSAAliasSets.h:43<br>
+<br>
+ class SingleMustAliasVec {<br>
+ MemoryLocation Loc;<br>
----------------<br>
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".<br>
<br>
<br>
================<br>
Comment at: include/llvm/Transforms/Utils/<wbr>MemorySSAAliasSets.h:54<br>
+ bool HasAGoodStore = false;<br>
+ SingleMustAliasVec(<wbr>MemoryLocation loc) : Loc(loc), CanNeverPromote(false) {<br>
+ Vec.insert(const_cast<Value *>(Loc.Ptr));<br>
----------------<br>
You probably don't need the `CanNeverPromote(false)` bit.<br>
<br>
<br>
================<br>
Comment at: include/llvm/Transforms/Utils/<wbr>MemorySSAAliasSets.h:58<br>
+ void addMemoryLocation(Optional<<wbr>MemoryLocation> MemLoc) {<br>
+ if (MemLoc == None)<br>
+ return;<br>
----------------<br>
This will be a line shorter without an early return.<br>
<br>
<br>
================<br>
Comment at: include/llvm/Transforms/Utils/<wbr>MemorySSAAliasSets.h:86<br>
+ unsigned MemorySSATraversalPenalty = 0;<br>
+ unsigned CAP_NO_ALIAS_SETS = 30;<br>
+ unsigned CAP_MSSA_TRAVERSALS = 30;<br>
----------------<br>
These need to be `cl::opt` s<br>
<br>
You should also split this header into a .cpp file (and put the cl::opt s in that .cpp file).<br>
<br>
<br>
================<br>
Comment at: include/llvm/Transforms/Utils/<wbr>MemorySSAAliasSets.h:90<br>
+ void dump() {<br>
+ for (SingleMustAliasVec *Set : AllSets)<br>
+ Set->dump();<br>
----------------<br>
You should use `LLVM_ENABLE_DUMP` and `LLVM_DUMP_METHOD` for this. See the definition and declaration of e.g. `DominanceFrontierWrapperPass:<wbr>:dump()`<br>
<br>
<br>
================<br>
Comment at: include/llvm/Transforms/Utils/<wbr>MemorySSAAliasSets.h:96<br>
+ if(!L)<br>
+ return true;<br>
+ return L->isLoopInvariant(V);<br>
----------------<br>
Can be a ternary. Same for loopContains<br>
<br>
<br>
================<br>
Comment at: include/llvm/Transforms/Utils/<wbr>MemorySSAAliasSets.h:112<br>
+ MemoryLocation &MemLocJ) {<br>
+ if (MemLocI == None) {<br>
+ ModRefInfo ModRef = AA->getModRefInfo(InstrI, MemLocJ);<br>
----------------<br>
I'd structure this as:<br>
<br>
```<br>
if (MemLocI)<br>
return AA->alias(MemLocI.getValue(), MemLocJ);<br>
// Rest of the code<br>
```<br>
<br>
<br>
<br>
================<br>
Comment at: include/llvm/Transforms/Utils/<wbr>MemorySSAAliasSets.h:247<br>
+ // before their children in the worklist and process the worklist in order.<br>
+ SmallVector<DomTreeNode *, 16> Worklist = collectChildrenInLoop(N, L);<br>
+<br>
----------------<br>
In `loopContains` you've handled the case where `L` is `nullptr`, but you've not done the same here.<br>
<br>
<br>
================<br>
Comment at: include/llvm/Transforms/Utils/<wbr>MemorySSAAliasSets.h:253<br>
+ if (Accesses)<br>
+ for (auto &Acc : make_range(Accesses->begin(), Accesses->end())) {<br>
+ if (AllSets.size() > CAP_NO_ALIAS_SETS ||<br>
----------------<br>
Can this be `for (auto &Acc : *Accesses)`?<br>
<br>
<br>
================<br>
Comment at: include/llvm/Transforms/Utils/<wbr>MemorySSAAliasSets.h:260<br>
+ // Process uses<br>
+ const MemoryUse *Use = dyn_cast_or_null<MemoryUse>(&<wbr>Acc);<br>
+ if (Use)<br>
----------------<br>
You can do:<br>
<br>
```<br>
if (const auto *Use = dyn_cast_or_null<MemoryUse>(&<wbr>Acc))<br>
addToAliasSets(Use);<br>
```<br>
<br>
also, the second `if` can be an `else if`.<br>
<br>
<br>
================<br>
Comment at: include/llvm/Transforms/Utils/<wbr>MemorySSAAliasSets.h:284<br>
+<br>
+ void replace(Value *LoadI, Value *Replacement) {<br>
+ for (auto OneAliasSet : AllSets) {<br>
----------------<br>
Can you please give this a more obvious name, like `replaceMemoryLocation`?<br>
<br>
Also, the name `LoadI` is misleading -- it isn't the load instruction, but it is the memory location, right?<br>
<br>
<br>
<a href="https://reviews.llvm.org/D35741" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D35741</a><br>
<br>
<br>
<br>
</blockquote></div><br></div>