[PATCH] D41525: [MemorySSA] Allow reordering of loads that alias in the presence of volatile loads.

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv accepted this revision.
george.burgess.iv added a comment.
This revision is now accepted and ready to land.

Thanks for this! Just a few nitpicks.

I think this is the right way forward, though please wait until tomorrow to land this change, so people have the chance to chime in if they'd like to.



================
Comment at: lib/Analysis/MemorySSA.cpp:210
+    return false;
 
   // If a load is seq_cst, it cannot be moved above other loads. If its ordering
----------------
Since this code is pretty subtle, can we keep a comment here saying something like "Otherwise, volatile doesn't matter here. From the langref: 'optimizers may change the order of volatile operations relative to non-volatile operations.'"?


================
Comment at: lib/Analysis/MemorySSA.cpp:223
+    return false;
+  return true;
 }
----------------
Can we simplify this to `return !(SeqCstUse || MayClobberIsAcquire)` (or the `!A && !B` equivalent; whatever you feel is cleaner)?


================
Comment at: lib/Analysis/MemorySSA.cpp:257
 
   if (auto *DefLoad = dyn_cast<LoadInst>(DefInst)) {
+    if (auto *UseLoad = dyn_cast<LoadInst>(UseInst))
----------------
Remove these braces, too, please.


https://reviews.llvm.org/D41525