Page MenuHomePhabricator

[AMDGPU] Make ds fp atomics overloadable
ClosedPublic

Authored by rampitec on Sep 18 2020, 2:43 PM.

Diff Detail

Event Timeline

rampitec created this revision.Sep 18 2020, 2:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 18 2020, 2:43 PM
Herald added subscribers: kerbowa, t-tye, tpr and 5 others. · View Herald Transcript
rampitec requested review of this revision.Sep 18 2020, 2:43 PM
arsenm added inline comments.Sep 18 2020, 4:07 PM
clang/lib/CodeGen/CGBuiltin.cpp
14772

I don't think you need a cast here (at least an addrspacecast)

rampitec marked an inline comment as done.Sep 18 2020, 4:09 PM
rampitec added inline comments.
clang/lib/CodeGen/CGBuiltin.cpp
14772

If removed builtins-amdgcn.cu fails. It is CUDA with LDS pointer passed as flat. I.e. it comes as cast from addrspace(3) to flat. Generic builtin handling below in this file does the same.

arsenm added inline comments.Sep 18 2020, 4:11 PM
clang/lib/CodeGen/CGBuiltin.cpp
14772

I thought these casts would be present in the AST?

rampitec marked an inline comment as done.Sep 18 2020, 4:13 PM
rampitec added inline comments.
clang/lib/CodeGen/CGBuiltin.cpp
14772

It comes as a flat pointer. I am just replicating what generic code does.

rampitec added inline comments.Sep 18 2020, 4:20 PM
clang/lib/CodeGen/CGBuiltin.cpp
14772

Check the code around the line 4440 in the same file. It does even more than that.

yaxunl accepted this revision.Wed, Sep 23, 11:37 AM
yaxunl added inline comments.
clang/lib/CodeGen/CGBuiltin.cpp
14772

There was a TargetInfo hook getCUDABuiltinAddressSpace which was introduced by Matt. The default implementation maps any address space to default address space 0. For some reason, it was not implemented as target specific to map the address space specified by builtin def to real ones. As a result, all builtin functions have generic pointer parameter for CUDA. Therefore the cast is needed here when calling the intrinsic.

We could consider fix that. For this patch, I think we still need the cast.

This revision is now accepted and ready to land.Wed, Sep 23, 11:37 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptWed, Sep 23, 11:40 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript