Page MenuHomePhabricator

Introduce infrastructure for an incremental port of SelectionDAG atomic load/store handling
ClosedPublic

Authored by reames on Aug 15 2019, 1:01 PM.

Details

Summary

This is the first patch in a large sequence. The eventual goal is to have unordered atomic loads and stores - and possibly ordered atomics as well - handled through the normal ISEL codepaths for loads and stores. Today, there handled w/instances of AtomicSDNodes. The result of which is that all transforms need to be duplicated to work for unordered atomics. The benefit of the current design is that it's harder to introduce a silent miscompile by adding an transform which forgets about atomicity.

Note that this patch is NFC unless the experimental flag is set.

The basic strategy I plan on taking is:

  1. introduce infrastructure and a flag for testing (this patch)
  2. Audit uses of isVolatile, and apply isAtomic conservatively*
  3. piecemeal conservative* update generic code and x86 backedge code in individual reviews w/tests for cases which didn't check volatile, but can be found with inspection
  4. flip the flag at the end (with minimal diffs)
  5. Work through todo list identified in (2) and (3) exposing performance ops

(*) The "conservative" bit here is aimed at minimizing the number of diffs involved in (4). Ideally, there'd be none. In practice, getting it down to something reviewable by a human is the actual goal. Note that there are (currently) no paths which produce LoadSDNode or StoreSDNode with atomic MMOs, so we don't need to worry about preserving any behaviour there.

We've taken a very similar strategy twice before with success - once at IR level, and once at the MI level (post ISEL). I'll probably need some help with some of the ISEL patterns since that's the part I'm not familiar with.

Next two patches for (2) above are posted as: D66318 (generic CodeGen) and D66322 (X86). I'll stop there until some of these make it through review.

Diff Detail

Repository
rL LLVM

Event Timeline

reames created this revision.Aug 15 2019, 1:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 15 2019, 1:01 PM
reames edited the summary of this revision. (Show Details)Aug 15 2019, 3:57 PM
reames edited the summary of this revision. (Show Details)Aug 15 2019, 4:46 PM
arsenm added a subscriber: arsenm.Aug 18 2019, 12:05 PM

This will help atomic store handling for global isel. Right now there's a complication because ISD::ATOMIC_STORE swaps the operand order from a normal store

Is there an llvm-dev thread for the general project? It looks fine to me, but maybe it should have a wider audience.

Are you planning to move all atomic load/store operations to this framework, eventually, or just unordered ones?

Do you have any test coverage for code using the flags from an unordered load folded into arithmetic, or a folded RMW operation?

craig.topper added inline comments.Fri, Aug 23, 5:49 PM
test/CodeGen/X86/atomic-unordered.ll
4 ↗(On Diff #215458)

Can we add a common prefix to share with the two O3 run lines? Looks like our codegen is the same for the majority of the test cases.

reames added a comment.EditedWed, Aug 28, 9:28 AM

Is there an llvm-dev thread for the general project? It looks fine to me, but maybe it should have a wider audience.

Will do. Sent as "FYI: proposed changes to atomic load/store in SelectionDAG" on Aug 28th.

Are you planning to move all atomic load/store operations to this framework, eventually, or just unordered ones?

Honestly, haven't decided. My near term needs are for unordered only.

Do you have any test coverage for code using the flags from an unordered load folded into arithmetic, or a folded RMW operation?

See test/CodeGen/X86/atomic-unordered.ll @rmw_fold_* for the later.
Will add the flag checks, I don't have those (I think).

reames marked an inline comment as done.Wed, Aug 28, 9:30 AM
reames added inline comments.
test/CodeGen/X86/atomic-unordered.ll
4 ↗(On Diff #215458)

Not quite following the why behind this request? Is there some (undocumented) feature of FileCheck which merges the check lines or something?

xbolva00 added inline comments.
test/CodeGen/X86/atomic-unordered.ll
4 ↗(On Diff #215458)

Possibly something like
-check-prefixes=CHECK,CHECK-O3
-check-prefixes=CHECK,CHECK-EX

(not tested, just saw it many times)

craig.topper added inline comments.Wed, Aug 28, 10:23 AM
test/CodeGen/X86/atomic-unordered.ll
4 ↗(On Diff #215458)

Yeah that's it. The test update script understands this. FileCheck and the update script also understand "--check-prefix=CHECK --check-prefix=CHECK-O3" on the same command line as meaning the same thing. FileCheck will check any lines with the listed prefix. For the script to work, the common prefix should be placed first and more specific prefix placed last. Then it will favor the common prefix when possible.

reames marked an inline comment as done.Wed, Aug 28, 1:19 PM
reames added inline comments.
test/CodeGen/X86/atomic-unordered.ll
4 ↗(On Diff #215458)

Did not know about this, cool.

reames added a comment.Tue, Sep 3, 1:58 PM

Ping. I'd like to get this landed if possible.

craig.topper added inline comments.Tue, Sep 3, 2:07 PM
test/CodeGen/X86/atomic-unordered.ll
4 ↗(On Diff #215458)

This test didn't get rebased and updated to merge the CHECK prefixes

reames updated this revision to Diff 218788.Wed, Sep 4, 2:08 PM

Rebase tests...

RKSimon added inline comments.Fri, Sep 6, 3:54 AM
include/llvm/CodeGen/TargetLowering.h
3719 ↗(On Diff #218788)

missing assertion message

3728 ↗(On Diff #218788)

missing assertion message

reames marked an inline comment as done.Fri, Sep 6, 9:30 AM
reames added inline comments.
include/llvm/CodeGen/TargetLowering.h
3719 ↗(On Diff #218788)

Happy to add something, but given the naming of the functions, I don't really think (... && "violated precondition") adds anything valuable.

This also feels like a very small nit. Is there any chance I can get an LGTM and just fix this before the commit? Refreshing this is a real pain since I have to rebuild the world.

This revision is now accepted and ready to land.Fri, Sep 6, 9:38 AM
RKSimon accepted this revision.Sat, Sep 7, 7:43 AM

LGTM, sorry for sidetracking the patch

This revision was automatically updated to reflect the committed changes.