<div dir="ltr">+Philip to get his input too.<div>I've talked with George offline, and here's a summary:<br><div><br></div><div>In <a href="https://reviews.llvm.org/D16875">D16875</a>, the decision made was: "<span style="color:rgb(0,0,0);font-family:"Segoe UI","Segoe UI Emoji","Segoe UI Symbol",Lato,"Helvetica Neue",Helvetica,Arial,sans-serif;font-size:13px">The LLVM spec is ambiguous about whether we can hoist a non-volatile load above a volatile load when the loads alias. It's probably best not to exploit this ambiguity at the moment by unconditionally allowing the motion of nonvolatile loads above volatile loads (and vice versa)"</span></div></div><div><span style="color:rgb(0,0,0);font-family:"Segoe UI","Segoe UI Emoji","Segoe UI Symbol",Lato,"Helvetica Neue",Helvetica,Arial,sans-serif;font-size:13px">So the testcase: </span><span style="font-size:12.8px">test/Analysis/MemorySSA/</span><wbr style="font-size:12.8px"><span style="font-size:12.8px">volatile-clobber.ll, is checking that a volatile load is the defining access of a load with which it may alias.</span></div><div><span style="font-size:12.8px"><br></span></div><div><span style="font-size:12.8px">Snippet:</span></div><div><div><span style="font-size:12.8px">; Ensuring that we don't automatically hoist nonvolatile loads around volatile</span></div><div><span style="font-size:12.8px">; loads</span></div><div><span style="font-size:12.8px">; CHECK-LABEL define void @volatile_only</span></div><div><span style="font-size:12.8px">define void @volatile_only(i32* %arg1, i32* %arg2) {</span></div></div><div><span style="font-size:12.8px">[...]</span></div><div><div><span style="font-size:12.8px">; MayAlias</span></div><div><span style="font-size:12.8px">; CHECK: 2 = MemoryDef(1)</span></div><div><span style="font-size:12.8px">; CHECK-NEXT: load volatile i32, i32* %arg1</span></div><div><span style="font-size:12.8px">  load volatile i32, i32* %arg1</span></div><div><span style="font-size:12.8px">; CHECK: MemoryUse(2)</span></div><div><span style="font-size:12.8px">; CHECK-NEXT: load i32, i32* %arg2</span></div><div><span style="font-size:12.8px">  load i32, i32* %arg2</span></div></div><div><div><span style="font-size:12.8px">  ret void</span></div><div><span style="font-size:12.8px">}</span></div></div><div><span style="font-size:12.8px"><br></span></div><div><span style="font-size:12.8px">The testcase</span>: test/Transforms/LICM/volatile-alias.ll <span style="font-size:12.8px">checks the opposite, that we *do* hoist the non-volatile load. This is currently ensured by the AliasSetTracker.</span></div><div><span style="font-size:12.8px"><br></span></div><div><span style="font-size:12.8px"><br></span></div><div><span style="font-size:12.8px">The conclusion I'm drawing is that right now AliasSetTracker and MemorySSA have different behaviors and replacing one with the either will naturally lead to different outcomes. </span></div><div><span style="font-size:12.8px">So, how can I make progress here?</span></div><div><br></div><div><span style="font-size:12.8px"><br></span></div><div>I think it's reasonable in <a href="https://reviews.llvm.org/D40375" style="font-size:12.8px">D40375</a><span style="font-size:12.8px"> to have</span> pointerInvalidatedByLoopWithMSSA only check if the defining access is within the current loop or liveOnEntry, and rely on MemorySSA to either consider a volatile load a clobbering access or not. So, right now the LICM/volatile-alias.ll testcase will behave differently with MemorySSA enabled.</div><div><br></div>A separate decision, is whether to update getLoadReorderability in MemorySSA to remove the restriction that loads should not alias. That would give the same behavior as the AliasSetTracker.<div><span style="font-size:13px;color:rgb(0,0,0);font-family:"Segoe UI","Segoe UI Emoji","Segoe UI Symbol",Lato,"Helvetica Neue",Helvetica,Arial,sans-serif">George's </span><span style="font-size:12.8px">recollection is that the reason MemorySSA didn't reorder aggressively is because it was untested at the time. Now that MemorySSA is used more widely, it may be a good time to revisit the initial decision?</span></div><div><span style="font-size:12.8px"><br></span></div><div><span style="font-size:12.8px">Thoughts?</span></div><div><span style="font-size:12.8px"><br></span></div><div><span style="font-size:12.8px"><br></span></div><div><span style="font-size:12.8px">Thanks,</span></div><div><span style="font-size:12.8px">Alina<br></span><div><div><span style="color:rgb(0,0,0);font-family:"Segoe UI","Segoe UI Emoji","Segoe UI Symbol",Lato,"Helvetica Neue",Helvetica,Arial,sans-serif;font-size:13px"><br></span></div></div></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Dec 20, 2017 at 11:50 AM, Krzysztof Parzyszek <span dir="ltr"><<a href="mailto:kparzysz@codeaurora.org" target="_blank">kparzysz@codeaurora.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On 12/20/2017 1:37 PM, Sanjoy Das wrote:><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Fwiw, I was under the impression that regular loads could *not* be<br>
reordered with volatile loads since we could have e.g.:<br>
<br>
   int *normal = &global_variable;<br>
   volatile int* ptr = 0;<br>
   int k = *ptr; // segfaults, and the signal handler writes to *normal<br>
   int value = *normal;<br>
<br>
and that we'd have to treat volatile loads and stores essentially as<br>
calls to unknown functions.<br>
</blockquote>
<br></span>
For this to work, "normal" should be volatile as well.<span class="HOEnZb"><font color="#888888"><br>
<br>
-Krzysztof</font></span><div class="HOEnZb"><div class="h5"><br>
<br>
-- <br>
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation<br>
</div></div></blockquote></div><br></div>