Page MenuHomePhabricator

[SelectionDAG] Enable lowering unordered atomics loads w/LoadSDNode (and stores w/StoreSDNode) by default
ClosedPublic

Authored by reames on Oct 19 2019, 2:38 PM.

Details

Summary

At this point, the experimental lowering introduced w/r371441 appears to be solidly correct - it's survived a good amount of fuzzing and a manual audit of differences in a fairly large corpus - and generated code which is mostly better than the current default. (There are a few cases which are worse in the test diffs, but nothing which looks concerning.)

As a reminder, the new lowering changes the representation of an unordered atomic load from an AtomicSDNode - which is essentially a black box which gets passed through without combines messing with it - to a LoadSDNode w/a atomic marker on the MMO. The later parallels the way we handle volatiles, and I've audited the code to ensure that every location which checks one checks the other.

Once this patch lands, the next steps are:

  1. Wait a week or two to ensure nothing falls out.
  2. Remove the experimental flag, consolidate some of the code in SelectionDAG as noted in existing todos, delete now stale td rules - i.e. prepare for this being the default long term
  3. Go through and implement a few of the individual TODOs (performance opportunities to improve ISEL) previously noted

Diff Detail

Event Timeline

reames created this revision.Oct 19 2019, 2:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 19 2019, 2:38 PM
arsenm accepted this revision.Mon, Oct 28, 11:15 AM
arsenm added a subscriber: arsenm.

LGTM

This revision is now accepted and ready to land.Mon, Oct 28, 11:15 AM
qcolombet accepted this revision.Mon, Oct 28, 11:56 AM
reames updated this revision to Diff 226749.Mon, Oct 28, 2:28 PM

It looks like I screwed up somewhere. I went to land the approached patch, ran make check and saw a bunch of unexpected failures.

Looking into the diffs thereof, it appears the new implementation is still buggy. We appear to be splitting an atomic load/store in some cases rather than emitting a library call. This is definitely incorrect.

As such, I'm considering the LGTMs here revoked, and will refresh this patch once issue is fixed.

Thanks Philip for double checking.

reames planned changes to this revision.Mon, Oct 28, 4:10 PM
reames updated this revision to Diff 226775.Mon, Oct 28, 4:13 PM

Add the missing type legalization, and asserts. I had failed to account for the fact that we don't expand atomic loads/stores which aren't natively legal, but can be represented as cmpxchg in IR, despite having a mechanism to do so.

This version looks to correct the bug in the test cases spotted, but I'm still nervous. There's code in LegalizeDAG.cpp (as opposed to type legalization) for ATOMIC_LOAD and ATOMIC_STORE which I don't have a parallel for, and which appears to be dead. (I added assert(false) and no x86 tests failed.) I don't really know what to do beyond that though.

This revision is now accepted and ready to land.Mon, Oct 28, 4:13 PM
reames requested review of this revision.Mon, Oct 28, 7:14 PM
arsenm accepted this revision.Mon, Oct 28, 7:22 PM

LGTM

This revision is now accepted and ready to land.Mon, Oct 28, 7:22 PM
reames closed this revision.Tue, Oct 29, 3:08 PM

This landed. For some reason the normal auto-close didn't trigger.

Author: Philip Reames
Date: 2019-10-29T12:46:24-07:00
New Revision: 2460989eabb8adca4b7973aa23997b5ec832219b

URL: https://github.com/llvm/llvm-project/commit/2460989eabb8adca4b7973aa23997b5ec832219b
DIFF: https://github.com/llvm/llvm-project/commit/2460989eabb8adca4b7973aa23997b5ec832219b.diff

LOG: [SelectionDAG] Enable lowering unordered atomics loads w/LoadSDNode (and stores w/StoreSDNode) by default

Enable the new SelectionDAG representation for unordered loads and stores introduced in r371441 by default. As a reminder, the new lowering changes the representation of an unordered atomic load from an AtomicSDNode - which is essentially a black box which gets passed through without combines messing with it - to a LoadSDNode w/a atomic marker on the MMO. The later parallels the way we handle volatiles, and I've audited the code to ensure that every location which checks one checks the other.

This has been fairly heavily fuzzed, and I examined diffs in a reasonable large corpus of assembly by hand, so I'm reasonable sure this is correct for the common case. Late in the review for this, it was discovered that I hadn't correctly handled cases which could be legalized into CAS operations. This points out that there's a strong bias in the IR of the frontend I'm working with towards only legal atomics. If there are problems with this patch, the most likely area will be legalization.

Differential Revision: https://reviews.llvm.org/D69219