Philip Reames via llvm-dev
2019-Feb-27 18:38 UTC
[llvm-dev] PSA: Changes to how atomics are handled in backends
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.
Philip Reames via llvm-dev
2019-Feb-27 20:24 UTC
[llvm-dev] PSA: Changes to how atomics are handled in backends
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