Page MenuHomePhabricator

[LegalizeTypes][RISCV] Correctly sign-extend comparison for ATOMIC_CMP_XCHG

Authored by jrtc27 on Feb 11 2020, 5:13 PM.



Currently, the comparison argument used for ATOMIC_CMP_XCHG is legalised
with GetPromotedInteger, which leaves the upper bits of the value
undefind. Since this is used for comparing in an LR/SC loop with a
full-width comparison, we must sign extend it. We introduce a new
getExtendForAtomicCmpSwapArg to complement getExtendForAtomicOps, since
many targets have compare-and-swap instructions (or pseudos) that
correctly handle an any-extend input, and the existing function
determines the extension of the result, whereas we are concerned with
the input.

This is related to, which solved the
issue for ATOMIC_CMP_SWAP_WITH_SUCCESS, but not the simpler

Diff Detail

Event Timeline

jrtc27 created this revision.Feb 11 2020, 5:13 PM
jrtc27 updated this revision to Diff 244040.Feb 11 2020, 5:17 PM

Removed atomic-rmw.ll change; nothing actually changed in there other than comments and a missing . on labels, so that churn can be postponed until it actually gets touched in a meaningful way.

Is there a reason this hasn't caused code changes for i16 or i8?

Is there a reason this hasn't caused code changes for i16 or i8?

There is no lr.b/lr.h, so they use the masked intrinsics, and hence everything is zero-extended already.

asb accepted this revision.Feb 20 2020, 5:28 AM

Great fix, thanks @jrtc27. Please do go ahead and commit.

This revision is now accepted and ready to land.Feb 20 2020, 5:28 AM

As this is such a simple change and fixes a nasty code generation bug, should this be backported to the release branch?

For our CHERI version of FreeBSD, I changed the atomic.h header to use __atomic_foo instead of hand-written assembly and this causes freezes on startup without this change.
I am considering upstreaming this change to FreeBSD, so it would be nice if clang 10 had a working __atomic_compare_exchange for uint32_t.
If this is backported, I could just guard it with #if __clang_major__ < 10.

asb added a comment.Feb 20 2020, 7:51 AM

Yes, if @jrtc27 commits I'd be supportive of a cherry-pick to the release branch.

efriedma added inline comments.Feb 20 2020, 10:55 AM

Please add an explanation for why this needs to be separate from getExtendForAtomicOps.

Hey @jrtc27 . Is there anything blocking this patch being committed?

jrtc27 marked 2 inline comments as done.Apr 1 2020, 7:52 AM

Hey @jrtc27 . Is there anything blocking this patch being committed?

No, I forgot about this/was busy with other things and this got neglected. Now committed.


Added in committed version.

This revision was automatically updated to reflect the committed changes.
jrtc27 marked an inline comment as done.