[PATCH] D11434: Allow value forwarding past release fences in EarlyCSE

[JF Bastien via llvm-commits]

jfb added inline comments.

================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:617
@@ +616,3 @@
+    // A release fence requires that all stores complete before it, but does
+    // not prevent the reordering of following loads 'before' the fence.  As a
+    // result, we don't need to consider it as writing to memory and don't need
----------------
reames wrote:
> jfb wrote:
> > reames wrote:
> > > jfb wrote:
> > > > Getting re-acquainted with the code: it doesn't seem like this change exercises a test?
> > > Huh?  Not sure what you're asking.  The first two tests in the included file cover cases this triggers.
> > True, they check that store/fence/load and store/fence/store are handled properly with release.
> > 
> > What I mean is: the code changes, but all of the tests are *exactly the same*. If code changes but tests remains the same, then what changed? :-)
> I'm still not clear what you're trying to ask.  We remove a load from both test1 and test2 which is not removed without the patch.  (Well, at the time I wrote it.  Haven't checked today, but I don't see any obvious new additions.)  Can you clarify?
Ah... The diff is wrong :-)
fence.ll is new, but the current phabricator diff shows it as modified. It doesn't show modifications to test1 and test2, so it looks like they the same as current tip-of-tree.

Could you fix the diff?


http://reviews.llvm.org/D11434