This is an archive of the discontinued LLVM Phabricator instance.

[LLVM] Add a check if should cast atomic operations to integer type
ClosedPublic

Authored by tianshilei1992 on May 15 2022, 5:45 PM.

Details

Summary

Currently for atomic load, store, and rmw instructions, as long as the
operand is floating-point value, they are casted to integer. Nowadays many
targets can actually support part of atomic operations with floating-point
operands. For example, NVPTX supports atomic load and store of floating-point
values. This patch adds a series interface functions shouldCastAtomicXXXInIR,
and the default implementations are same as what we currently do. Later for
targets can have their specialization.

Diff Detail

Event Timeline

tianshilei1992 created this revision.May 15 2022, 5:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 15 2022, 5:45 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
tianshilei1992 requested review of this revision.May 15 2022, 5:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 15 2022, 5:45 PM
This revision is now accepted and ready to land.May 16 2022, 7:17 AM
tra added a subscriber: tra.May 16 2022, 4:09 PM
tra added inline comments.
llvm/include/llvm/CodeGen/TargetLowering.h
2050

just cast. It's an irregular verb.

2052

Would it make sense to generalize all these into should{Cast,Expand}AtomicOpInIR(Instruction *I) so we don't have to encode each instruction variant into the function name?

rebase and fix comment

tianshilei1992 marked 2 inline comments as done.May 17 2022, 8:06 PM
tianshilei1992 added inline comments.
llvm/include/llvm/CodeGen/TargetLowering.h
2052

Sounds like a good idea. I'll do it in another patch.

This revision was landed with ongoing or failed builds.May 20 2022, 2:24 PM
This revision was automatically updated to reflect the committed changes.
tianshilei1992 marked an inline comment as done.

As somewhat coincidentally just discussed in the thread on https://reviews.llvm.org/D124728 the target hooks don't really make sense as implemented here.

Whether a cast to an integer is required is due in part to what code AtomicExpandPass itself generates (configured based upon existing target hooks), and whether _that_ expansion requires the value to be an integer (almost all of them currently do). Requiring targets to deduce those requirements and encode it into a brand new target hook seems error-prone, and should be avoided.

@efriedma

tkf added a subscriber: tkf.May 25 2022, 8:07 PM