[llvm-dev] PSA: Changes to how atomics are handled in backends

[Philip Reames via llvm-dev]

This message is for anyone who maintains an out of tree backend, and who
cares about supporting atomics.  Be aware that default behavior is
changing, and action on your part is required to preserve legacy
behavior.  For other readers, you may find the design summary at the
bottom interesting if you're interested in how we handle atomics in the
backends. 

In D57601, I am adjusting SelectionDAG such that atomic, but
non-volatile, operations are no longer marked as volatile as well.  I
will be landing this change today, and will reply to this thread with
the commit ID for ease of discovery.

The result of this is that out of tree backends may need updated in one
of two ways.

Option 1 (Recommended) - Audit locations which use isVolatile() on
either a MachineMemOperand, or the MachineInstruction.  In the short
term, update each to also check isAtomic() to preserve old behavior.  If
you backend lowers AtomicSDNodes to (non-atomic) LoadSDNode or
StoreSDNode w/in SelectionDAG, then you also need to set the MOVolatile
flag on the resulting node. 

Option 2 - Opt out of the change for your backend by overriding the
getMMOFlags, and returning MOVolatile for the instructions who's MMOs
you wish to poison.  See the review for an example of what this code
looks like in XCore, and SystemZ.  

Philip

p.s. For anyone who's curious, here's a bit more technical detail and
background. 

We previously always marked atomic operations as being volatile as well
in SelectionDAG and thus, later MI.  This was represented by setting
both the ordering, and the MOVolatile flag on the corresponding MMOs. 
In SelectionDAG, we also have distinct families of nodes for atomic and
non-atomic loads and stores.

The current change does not remove the split between atomic and
non-atomic node types in SelectionDAG, but does adjust the formation of
the AtomicSDNodes such that the MOVolatile flag is only set if the
operation is actually volatile. 

I've landed a set of changes (listed in the review and commit) which
adjusted common code and upstream backend code to treat atomic MMOs as
conservatively as volatile ones.  As a result, removing the MOVolatile
flag should be strictly NFC for code generated by SelectionDAG.

The assumption is that all DAG combines do not need audited because we
never see a LoadSDNode with an MMO marked atomic (but not volatile). 
This is true today, and should remain true after this change.  As noted
above, downstream backends may need adjusted to ensure this.

Very few backends handle atomics in FastISel.  The one exception I found
was AArch64, and technically, I may have regressed optimizations for
non-volatile release stores w/previously committed changes.  However,
there are no tests for this, so I doubt anyone actually noticed.  I just
noticed this now, but if anyone has a test case which actually suffers,
please let me know and I'll fix it ASAP. 

No backend I could find handles atomics in GlobalISel.  I plan to extend
x86 GlobalISel w/atomic load and store support in the not too distant
future.

My intention is to let this change bake for a week or two before taking
any following action.  Once I'm reasonably sure no nasty bugs were
lurking - or have fixed them - I plan to work through the isAtomic
bailouts added in previous changes one by one to teach the X86 backend
(and thus common backend code) to optimize atomic, but non-volatile, MI
instructions were legal.  Once that's done, I *may* return to the split
representation in SelectionDAG, but I'm putting that off as long as I can.