This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen] Be conservative about atomic accesses as for volatile
ClosedPublic

Authored by reames on Feb 1 2019, 9:45 AM.

Details

Summary

Background: At the moment, we record the AtomicOrdering of an access in the MMO, but also mark any atomic access as volatile in SelectionDAG. I'm working towards separating that. See https://reviews.llvm.org/D57601 for context.

Update all usages of isVolatile in lib/CodeGen to preserve behaviour once atomic MMOs stop being also volatile. This is NFC in it's current form, but is essential for correctness once we make that final change.

It useful to keep in mind that AtomicSDNode is not a parent of LoadSDNode, StoreSDNode, or LSBaseSDNode. As a result, any call to isVolatile on one of those static types doesn't need a companion isAtomic check.

I'm deliberately being conservative about handling. I want the change to stop adding volatile to be NFC itself, and then will work through places where we can be less conservative for atomics one by one in separate changes w/tests.

Diff Detail

Event Timeline

reames created this revision.Feb 1 2019, 9:45 AM

My main comment is, in most or all of these places where we s/isVolatile/isVolatile && isAtomic/, it's not going to be obvious to a reader whether isAtomic is really necessary, or just part of a conservative check.

I see that you plan to go back over these, and that's cool, but I wonder if we should leave a breadcrumb that indicates that you plan to come back to this. That way if you miss a spot or whatever, readers won't think that isAtomic is somehow deeply meaningful (and won't have to scour through blame to find this comment I'm leaving now... :).

Even something as simple as //TODO(reames): Figure out whether isAtomic is really necessary (see D57601).

include/llvm/CodeGen/SelectionDAGNodes.h
69 ↗(On Diff #184774)

Nit: Is this comment worth the cost?

reames planned changes to this revision.Feb 1 2019, 11:20 AM

My main comment is, in most or all of these places where we s/isVolatile/isVolatile && isAtomic/, it's not going to be obvious to a reader whether isAtomic is really necessary, or just part of a conservative check.

I see that you plan to go back over these, and that's cool, but I wonder if we should leave a breadcrumb that indicates that you plan to come back to this. That way if you miss a spot or whatever, readers won't think that isAtomic is somehow deeply meaningful (and won't have to scour through blame to find this comment I'm leaving now... :).

Even something as simple as //TODO(reames): Figure out whether isAtomic is really necessary (see D57601).

Sure. I won't include the name since that's not idiomatic, but I'll add comments and update.

reames updated this revision to Diff 184850.Feb 1 2019, 2:44 PM

Update w/requested todos and a simplification. It occurred to me that LoadSDNode is not a subclass of AtomicSDNode, so anything of static type LoadSDNode doesn't need the conservative atomicity check.

jlebar accepted this revision.Feb 1 2019, 2:48 PM
This revision is now accepted and ready to land.Feb 1 2019, 2:48 PM
reames edited the summary of this revision. (Show Details)Feb 1 2019, 2:49 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2019, 2:58 PM