This revision was automatically updated to reflect the committed changes.
Closed by commit rC332848: [AMDGPU] fixes for lds f32 builtins (authored by
dfukalov, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D43281?vs=142412=147801#toc
Repository:
rC Clang
arsenm added a comment.
I think the intent of the current code is for the address space to correspond
to a "target address space" as if the user code used
__attribute__((address_space(n))) to specify a pointer value. This is
confusingly named, and different from the target address space
arsenm added a comment.
I'm looking at how the address space mapping works for builtins, and I think
what's there is just uselessly broken and needs to be fixed. It seems to be
operating under the assumption that the address spaces the target defines are
totally disjoint from the language
yaxunl accepted this revision.
yaxunl added a comment.
This revision is now accepted and ready to land.
LGTM. Thanks!
Repository:
rC Clang
https://reviews.llvm.org/D43281
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
dfukalov updated this revision to Diff 142412.
dfukalov edited the summary of this revision.
Repository:
rC Clang
https://reviews.llvm.org/D43281
Files:
include/clang/Basic/BuiltinsAMDGPU.def
lib/CodeGen/CGBuiltin.cpp
test/CodeGenOpenCL/builtins-amdgcn-vi.cl
dfukalov added a comment.
ping...
Repository:
rC Clang
https://reviews.llvm.org/D43281
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
dfukalov added a comment.
ping...
Repository:
rC Clang
https://reviews.llvm.org/D43281
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
dfukalov added a comment.
> My real question was what happens if you put 11 in the description string?
in this case CanT.getAddressSpace() returns target addrspace value "20" (also
shifted in the enum by 9==LangAS::FirstTargetAddressSpace)
So again ASTContext::getAddrSpaceQualType decieds that
arsenm added a comment.
In https://reviews.llvm.org/D43281#1023962, @dfukalov wrote:
> The problem is that if set addrspace "2" in description string,
> CanT.getAddressSpace() returns target addrspace value "11" (shifted in the
> enum) and compares it with input LangAS addrspace ("2",
dfukalov added a comment.
ping...
Repository:
rC Clang
https://reviews.llvm.org/D43281
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
dfukalov updated this revision to Diff 136752.
dfukalov edited the summary of this revision.
dfukalov set the repository for this revision to rC Clang.
dfukalov added a comment.
addrspace specifications are kept in descriptions strings
Repository:
rC Clang
https://reviews.llvm.org/D43281
dfukalov added a comment.
The problem is that if set addrspace "2" in description string,
CanT.getAddressSpace() returns target addrspace value "11" (shifted in the
enum) and compares it with input LangAS addrspace ("2", "opencl_local" in our
case).
So I cannot set a number a description
arsenm added a comment.
I meant we could just change the value to whatever it happens to be for AMDGPU.
That this is the language address space is surprising to me though, so maybe
that should change
https://reviews.llvm.org/D43281
___
dfukalov added a comment.
In https://reviews.llvm.org/D43281#1018657, @arsenm wrote:
> Can’t you just change the description to be the LangAS value? I also thought
> these happened to be the same already
Am I right that you mean to change the semantic of the addrspace number in a
description
arsenm added a comment.
Can’t you just change the description to be the LangAS value? I also thought
these happened to be the same already
https://reviews.llvm.org/D43281
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
dfukalov marked an inline comment as done.
dfukalov added a comment.
ping...
https://reviews.llvm.org/D43281
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
dfukalov marked an inline comment as done.
dfukalov added inline comments.
Comment at: lib/CodeGen/CGBuiltin.cpp:9866
+ case AMDGPU::BI__builtin_amdgcn_ds_fmax: {
+llvm::SmallVector Args;
+for (unsigned I = 0; I != 5; ++I)
b-sumner wrote:
> Can the
dfukalov updated this revision to Diff 134503.
dfukalov edited the summary of this revision.
dfukalov added a comment.
diff updated as requested by reviewer
https://reviews.llvm.org/D43281
Files:
include/clang/Basic/BuiltinsAMDGPU.def
lib/CodeGen/CGBuiltin.cpp
b-sumner added inline comments.
Comment at: lib/CodeGen/CGBuiltin.cpp:9866
+ case AMDGPU::BI__builtin_amdgcn_ds_fmax: {
+llvm::SmallVector Args;
+for (unsigned I = 0; I != 5; ++I)
Can the pointer argument address space be checked here?
Repository:
dfukalov created this revision.
dfukalov added reviewers: b-sumner, arsenm.
dfukalov added a project: AMDGPU.
Herald added subscribers: cfe-commits, t-tye, tpr, dstuttard, yaxunl, nhaehnle,
wdng, kzhuravl.
1. removed addrspace 3 specifications from builtins description strings since
it's not
20 matches
Mail list logo
