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

[Philip Reames via llvm-dev]

This landed as revision 355025. 

On 2/27/19 10:38 AM, Philip Reames via llvm-dev wrote:
> 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. 
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev