LGTM, there is just an assert to fix.

It sounds great to me (although a lot of work for such a corner case, but I don't see any better solution).

After thinking some more about it, I realized that having the load stay non-atomic is actually unsound because there might be a race, which would make it undefined behaviour.
Sadly, making it monotonic would still introduce these redundant cmpxchg8b/16b since monotonic requires the access to be atomic.
I don't know well enough the semantics of unordered accesses to know if they would do the trick, and the llvm reference manual description of them is fairly lightweight on details.

In D12338#232597, @jfb wrote:

This isn't just for PNaCl, so the description should be updated. You should mention something along the lines of:

On weak memory systems the CPU can speculate on subsequent loads (e.g. the cmpxchg) and observe them without honoring the happens-before ordering of the corresponding stores. This is the "load buffering" problem in literature, and occurs on ARM and POWER.

LGTM for the integer part.

To dberlin: I looked at this article, but they explain well that the reason min-cut is so expensive for PRE is because it must be repeated for each computation in the function (of which there can be 10s of thousand in very large function) and must look at a potentially huge graph. In comparison we only run this twice: once for hwsync and one for lwsync. Furthermore, because the graph is stopped by any memory access (and not just use/kill of some very specific computation as in PRE), I expect each of these runs of min-cut to be quite cheap. I have not had the time to benchmark the compile-time cost of this pass (deadline tomorrow for PLDI..), but in summary I expect it to be small, even for large functions full of fences.

Hi Hal,

ping.

Initialize the FlowGraphNode::Next iterator more cleanly

Add an option "-view-fences-pre-flow-graph" to visualize the graph being used,
per the request of jfb (it makes things much easier to debug).

Closed by commit rL219957 (authored by @morisset).

Apply clang-format, fix small style issues pointed by jfb.
I haven't done the bigger modifications yet.

I missed your idea of running this pass for C++11 fences instead. In short: this is probably useless, as C++11 fences are expected to be rather rare (compared to atomic accesses that result in target-specific fences), and the semantics of C++11 fences are hairy enough I don't want to try without a clear benefit. So while the infrastructure could support it, I don't plan on doing it.

Thanks for the review !

I don't know enough about this topic to comment on the details of the ideas (although they seem reasonable and match what had been discussed on the mailing list), but I have a read part of the patch and found a few typos and have a few inline comments.

r219357

Follow jfb suggestion.

Closed by commit rL219336 (authored by @morisset).

Rebase on trunk + use doxygen trick suggested by jfb

Closed by commit rL219257 (authored by @morisset).

Apply jfb suggestions.

Based on an offline comment by jf, improve the patch so that the immediate is
merged into the addq.

Closed by commit rL218995 (authored by @morisset).

I will add the test with ppc440.

Closed by commit rL218937 (authored by @morisset).

Rebase on trunk.

Duplicate with commit r218923 by Hal FInkel.

Closed by commit rL218922 (authored by @morisset).

Add tests for indexed loads/stores

Thank you ! This paragraph of the documentation was indeed what I was
thinking about, when saying that atomicity is not a problem, I should have
made it more explicit. I will try to generate tests for the indexed
versions and add them in.

I don't think I know this code well enough to give the LGTM, but the general approach looks good.
Also, thank you for the detailed testing/explanation.

Closed by commit rL218455 (authored by @morisset).

Closed by commit rL218332 (authored by @morisset).

Add requested comments

Closed by commit rL218331 (authored by @morisset).

Closed by commit rL218329 (authored by @morisset).

Partial cleanup of the control-flow + extra test + fixed formatting/typos.

Thanks for the review. Answers inline, and patch next.

Use CHECK-LABEL and CHECK-NEXT

Preserve FIXME comment, and refine tests.

The tests for cmpxchg16b were indeed testing for the lock prefix, but not the tests for the 8b version. I will upload a new patch with an improved test (and preserved FIXME).

Updated comments.

Thanks for the review, I will fix the comments as described below.

Doing this optimization so late was looking increasingly brittle as it must interact with AtomicExpandPass earlier at the IR level.
I reimplemented it at that level in D5422, and it turned out a lot cleaner.

Cleanup of some now redundant code that I had missed.

Closed by commit rL218066 (authored by @morisset).

Closed by commit rL217965 (authored by @morisset).

Rebase on trunk.

There is no barrier generated by the other path, the entire operation (store/load/rmw/whatever) is turned into a libcall on these old processors. It is tested (summarily) at the end of atomic-load-store.ll at least.

r217928

It mostly seems reasonable to me, but I don't know this area of the code, and do not feel at all comfortable giving an LGTM, so someone more experienced should look at it.

Thanks !
I will commit it soon (currently testing after rebase on trunk).

ping. Is this patch good to go ? There are several other patches with dependencies on it (such as D5179 - also needing review, and D5180 - that already received an LGTM).

Thanks jf for catching this forgotten test!

I did not know that ppc is also supposed to be a generic processor. I can
either add HasSYNC to it, or remove it from generic (and change the broken
tests). Which would you prefer ?

I had originally left HasSYNC out of generic, but added it in because it broke a bunch of tests (which did not specify anything about the processor and yet were expecting syncs). Should I instead modify all of these tests to use a given processor ? If so which one ?

Improve comments.

Fix typo and add comment.

Add a Fixme about the use of IsBookE flag for sync support.

Fix formatting based on hfinkel comments.

Closed by commit rL217080 (authored by @morisset).

r217076
Thanks everyone

Ok, can the revision be accepted then ? Thanks.

Fix wrong update

Update comment.

MakeDMB -> makeDMB
Erase comment.

Thanks for the review, I will modify the comments accordingly.

There was already a lot of tests for atomics on X86 (test/CodeGen/X86/atomic*), so I did not modify them in this patch, just checked that they still pass.

Thanks for the review !

ping.

Commited r216980

Got a lgtm from Nadav Rotem on the mailing list.

Closed by commit rL216940 (authored by @morisset).

Fix the formatting

Closed by commit rL216784 (authored by @morisset).