llvm dev - Aug 2019 - FYI: proposed changes to atomic load/store in SelectionDAG

I have a set of changes out for review which are possibly note worthy, 
and backend contributors may wish to be aware of.

TLDR: atomic loads as normal LoadSDNodes w/an "isAtomic" flag.


Background

At the moment, we lower all atomic loads and stores as instances of 
AtomicSDNode (along with cmpxchg, and atomicrmw).  This requires us to 
duplicate any isel rules we wish to apply for atomic loads or stores, 
but does have the nice property  that it's harder to introduce a silent 
miscompile by adding an transform which forgets about atomicity.


Proposed End Result

Represent atomic loads and stores as normal LoadSDNode or StoreSDNodes.  
Analogously to volatility, provide a flag on the node (stored in the 
MMO) which indicates whether the operation is atomic.  All transforms 
updated to check isAtomic if needed.

The advantages of this representations are:
1) Once the audit has been done, it makes it easier to keep atomic and 
non-atomic rules in sync.
2) It makes GlobalISEL easier (by eliminating the need for the special 
case).
3) Unify patterns flowing through other backend passes (i.e. unordered 
atomics and non-atomics shouldn't generate radically different MI 
structures)

One open question is whether we do this just for unordered atomics, or 
for all atomics.  I'd be open to either, but would start with just 
unordered to start with either way.


Migration Plan

This would be done on a per-backend basis, and to start with, I'm only 
proposing to port X86.

The basic strategy I plan on taking is:

 1. introduce infrastructure and a flag for testing
    (https://reviews.llvm.org/D66309)
 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 (x86) 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 
behavior 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.


Thoughts?

Philip

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190828/600e5448/attachment.html>

I think you nailed the major concern, which is that compilers tend to have
bugs where they accidentally fold atomic or volatile loads when they
shouldn't.

It sounds like you plan to do the audit and phase things in with a flag.
The flag gives me some confidence that downstream consumers will be able to
test this before it's turned on by default. So, if you're motivated
enough
to do the audit, go for it. :)

On Wed, Aug 28, 2019 at 1:40 PM Philip Reames via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> I have a set of changes out for review which are possibly note worthy, and
> backend contributors may wish to be aware of.
>
> TLDR: atomic loads as normal LoadSDNodes w/an "isAtomic" flag.
>
>
> Background
>
> At the moment, we lower all atomic loads and stores as instances of
> AtomicSDNode (along with cmpxchg, and atomicrmw).  This requires us to
> duplicate any isel rules we wish to apply for atomic loads or stores, but
> does have the nice property  that it's harder to introduce a silent
> miscompile by adding an transform which forgets about atomicity.
>
>
> Proposed End Result
>
> Represent atomic loads and stores as normal LoadSDNode or StoreSDNodes.
> Analogously to volatility, provide a flag on the node (stored in the MMO)
> which indicates whether the operation is atomic.  All transforms updated to
> check isAtomic if needed.
>
> The advantages of this representations are:
> 1) Once the audit has been done, it makes it easier to keep atomic and
> non-atomic rules in sync.
> 2) It makes GlobalISEL easier (by eliminating the need for the special
> case).
> 3) Unify patterns flowing through other backend passes (i.e. unordered
> atomics and non-atomics shouldn't generate radically different MI
> structures)
>
> One open question is whether we do this just for unordered atomics, or for
> all atomics.  I'd be open to either, but would start with just
unordered to
> start with either way.
>
>
> Migration Plan
>
> This would be done on a per-backend basis, and to start with, I'm only
> proposing to port X86.
>
> The basic strategy I plan on taking is:
>
>    1. introduce infrastructure and a flag for testing (
>    https://reviews.llvm.org/D66309)
>    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 (x86) 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 behavior 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.
>
>
> Thoughts?
>
> Philip
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190830/466c97cf/attachment.html>

All of the changes in this sequence have landed, and the default for X86 
has been flipped in git 246098.

There will be some cleanup work and further optimization work in a 
couple of weeks, but I'm going to give this a bit of burn-in period 
before doing anything else to give problems time to be reported.

Philip

On 8/28/19 1:40 PM, Philip Reames via llvm-dev wrote:
>
> I have a set of changes out for review which are possibly note worthy, 
> and backend contributors may wish to be aware of.
>
> TLDR: atomic loads as normal LoadSDNodes w/an "isAtomic" flag.
>
>
> Background
>
> At the moment, we lower all atomic loads and stores as instances of 
> AtomicSDNode (along with cmpxchg, and atomicrmw). This requires us to 
> duplicate any isel rules we wish to apply for atomic loads or stores, 
> but does have the nice property that it's harder to introduce a silent 
> miscompile by adding an transform which forgets about atomicity.
>
>
> Proposed End Result
>
> Represent atomic loads and stores as normal LoadSDNode or 
> StoreSDNodes.  Analogously to volatility, provide a flag on the node 
> (stored in the MMO) which indicates whether the operation is atomic.  
> All transforms updated to check isAtomic if needed.
>
> The advantages of this representations are:
> 1) Once the audit has been done, it makes it easier to keep atomic and 
> non-atomic rules in sync.
> 2) It makes GlobalISEL easier (by eliminating the need for the special 
> case).
> 3) Unify patterns flowing through other backend passes (i.e. unordered 
> atomics and non-atomics shouldn't generate radically different MI 
> structures)
>
> One open question is whether we do this just for unordered atomics, or 
> for all atomics.  I'd be open to either, but would start with just 
> unordered to start with either way.
>
>
> Migration Plan
>
> This would be done on a per-backend basis, and to start with, I'm only 
> proposing to port X86.
>
> The basic strategy I plan on taking is:
>
>  1. introduce infrastructure and a flag for testing
>     (https://reviews.llvm.org/D66309)
>  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 (x86) 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 behavior 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.
>
>
> Thoughts?
>
> Philip
>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191029/c45f1f9f/attachment.html>