Page MenuHomePhabricator

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

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

Details

Summary

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 https://reviews.llvm.org/D58829, which solved the
issue for ATOMIC_CMP_SWAP_WITH_SUCCESS, but not the simpler
ATOMIC_CMP_SWAP.

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
llvm/include/llvm/CodeGen/TargetLowering.h
1967

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.Wed, Apr 1, 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.

llvm/include/llvm/CodeGen/TargetLowering.h
1967

Added in committed version.

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