<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Aug 2, 2016 at 12:53 PM, Sebastian Pop <span dir="ltr"><<a href="mailto:sebpop@gmail.com" target="_blank">sebpop@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">sebpop added a subscriber: sebpop.<br>
<br>
================<br>
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1116<br>
@@ +1115,3 @@<br>
+ return CS.getCalledValue() == Other.CS.getCalledValue();<br>
+ else<br>
+ return Loc == Other.Loc;<br>
----------------<br>
No need for "else" after return.<br>
<br></blockquote><div>Fixed.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
================<br>
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1139<br>
@@ +1138,3 @@<br>
+ MLOC.getCS().getCalledValue()));<br>
+ else<br>
+ return hash_combine(<br>
----------------<br>
Remove "else".<br>
<br></blockquote><div>Fixed</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
================<br>
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1246<br>
@@ +1245,3 @@<br>
+ auto &LocInfo = LocStackInfo[UseMLOC];<br>
+ // If the pop epoch changed, if means we've removed stuff from top of<br>
+ // stack due to changing blocks. We may have to reset the lower bound or<br>
----------------<br>
s/if means/it means/<br>
<br></blockquote><div>Fixed.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
================<br>
Comment at: llvm/trunk/lib/Transforms/Utils/MemorySSA.cpp:1152<br>
@@ +1151,3 @@<br>
+/// is a batch walker that touches everything, it does not operate like the<br>
+/// other walkers. This walker is basically performing a top-down SSA renaming<br>
+/// pass, where the version stack is used as the cache. This enables it to be<br>
----------------<br>
Would it be possible to merge the UseOptimizer with the renamePass?<br></blockquote><div><br></div><div><br></div><div>No, because phi nodes are not guaranteed complete until after renaming is done.</div><div><br></div><div>(more particularly, there is no ordering in which to do renaming that can guarantee you don't hit incomplete phi nodes).</div><div><br></div><div>At one point, we had use optimization be part of renaming, but walking incomplete phi nodes is ....</div><div>complicated and not guaranteed to get right answers.</div><div> </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
================<br>
Comment at: llvm/trunk/lib/Transforms/Utils/MemorySSA.cpp:1284<br>
@@ +1283,3 @@<br>
<span class="">+ DEBUG(dbgs() << "We are being asked to check up to "<br>
</span>+ << UpperBound - LocInfo.LowerBound<br>
+ << " loads and stores, so we didn't.\n");<br>
----------------<br>
This message needs some rewording.<br></blockquote><div>Fixed.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
================<br>
Comment at: llvm/trunk/lib/Transforms/Utils/MemorySSA.cpp:1462<br>
@@ -1163,21 +1461,3 @@<br>
Walker->setAutoResetWalker(false);<br>
-<br>
- // Now optimize the MemoryUse's defining access to point to the nearest<br>
- // dominating clobbering def.<br>
- // This ensures that MemoryUse's that are killed by the same store are<br>
- // immediate users of that store, one of the invariants we guarantee.<br>
- for (auto DomNode : depth_first(DT)) {<br>
- BasicBlock *BB = DomNode->getBlock();<br>
- auto AI = PerBlockAccesses.find(BB);<br>
- if (AI == PerBlockAccesses.end())<br>
- continue;<br>
- AccessList *Accesses = AI->second.get();<br>
- for (auto &MA : *Accesses) {<br>
- if (auto *MU = dyn_cast<MemoryUse>(&MA)) {<br>
- Instruction *Inst = MU->getMemoryInst();<br>
- MU->setDefiningAccess(Walker->getClobberingMemoryAccess(Inst));<br>
- }<br>
- }<br>
- }<br>
-<br>
+ OptimizeUses(this, Walker, AA, DT).optimizeUses();<br>
Walker->setAutoResetWalker(true);<br>
----------------<br>
If the renamePass (called just above) gets the right answer, there is no need to OptimizeUses ;-)<br>
Maybe the logic from OptimizeUses can be merged in the renamePass.<br>
<div class="HOEnZb"><div class="h5"><br></div></div></blockquote><div>You can look back at the history of the review of the original patches to see a version where it is merged.<br></div><div>It turns out to be a really bad idea ;)</div><div><br></div></div></div></div>