This is an archive of the discontinued LLVM Phabricator instance.

GlobalISel: Don't fail translate on weak cmpxchg
ClosedPublic

Authored by arsenm on Jun 12 2020, 6:37 AM.

Details

Summary

The translation of cmpxchg added by
9481399c0fd2c198c81b92636c0dcff7d4c41df2 specifically skipped weak
cmpxchg due to not understanding the meaning. Weak cmpxchg was added
in 420a216817def01816186910a2e35885c9201951. As explained in the
commit message, the weak mode is implicit in how
ATOMIC_CMP_SWAP_WITH_SUCCESS is lowered. If it's expanded to a regular
ATOMIC_CMP_SWAP, it's replaced with a strong cmpxchg.

This handling seems weird to me, but this was already following the
DAG behavior. I would expect the strong IR instruction to not have the
boolean output. Failing that, I might expect the IRTranslator to emit
ATOMIC_CMP_SWAP and a constant for the boolean.

Diff Detail

Event Timeline

arsenm created this revision.Jun 12 2020, 6:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 12 2020, 6:37 AM
jfb added inline comments.Jun 12 2020, 8:23 AM
llvm/test/CodeGen/AArch64/GlobalISel/arm64-irtranslator.ll
1980

Does this now contains two loops instead of just one?

arsenm marked an inline comment as done.Jun 12 2020, 8:49 AM
arsenm added inline comments.
llvm/test/CodeGen/AArch64/GlobalISel/arm64-irtranslator.ll
1980

No? Not sure why you think there is

jfb added inline comments.Jun 12 2020, 9:14 AM
llvm/test/CodeGen/AArch64/GlobalISel/arm64-irtranslator.ll
1980

The label name is different. It's a fundamental property of weak cmpxchg that it's (mostly) used inside a user-provided loop, so it's important to make sure we don't add an inner loop when translating it.

arsenm marked an inline comment as done.Jun 12 2020, 9:16 AM
arsenm added inline comments.
llvm/test/CodeGen/AArch64/GlobalISel/arm64-irtranslator.ll
1980

The label name is the same, MIR functions just get bb.<blocknumber> prepended to the IR block name

paquette accepted this revision.Jun 26 2020, 10:40 AM

Considering this just follows the DAG behaviour, this LGTM.

This revision is now accepted and ready to land.Jun 26 2020, 10:40 AM