[PATCH] D21849: [AMDGPU] Add metadata for OpenCL runtime
Matt Arsenault via llvm-commits
llvm-commits at lists.llvm.org
Wed Jun 29 12:23:19 PDT 2016
arsenm added a comment.
Needs more tests. I would like the full set of types to be tested, particularly weird ones like 3-vectors. Other non-OpenCL types like i1 or a random other bitwidth should also be tested. A few tests that use multiple arguments also would be useful
================
Comment at: lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:46
@@ +45,3 @@
+namespace {
+ const char OPENCL_METADATA_SECTION[] = ".OpenCL.Metadata";
+}
----------------
Should not use macro naming convention
================
Comment at: lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:724
@@ +723,3 @@
+ unsigned N = 0;
+ for (auto &I:M.functions())
+ if (I.getMetadata("kernel_arg_type"))
----------------
Spaces around :
================
Comment at: lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:739-744
@@ +738,8 @@
+ }
+ if (Ty->isHalfTy())
+ return "half";
+ if (Ty->isFloatTy())
+ return "float";
+ if (Ty->isDoubleTy())
+ return "double";
+ if (!isSigned)
----------------
You could also switch over the TypeID
================
Comment at: lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:758
@@ +757,3 @@
+ default:
+ llvm_unreachable("invalid integer type");
+ }
----------------
This is reachable with valid IR. How about returning an empty string, or i + bitwidth?
================
Comment at: lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:776
@@ +775,3 @@
+
+ enum KernelArgType : char{
+ KAT_Pointer = 0,
----------------
Missing space before {
================
Comment at: lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:777-781
@@ +776,7 @@
+ enum KernelArgType : char{
+ KAT_Pointer = 0,
+ KAT_Value = 1,
+ KAT_Image = 2,
+ KAT_Sampler = 3,
+ KAT_Queue = 4,
+ };
----------------
These enum should be moved to a header
================
Comment at: lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:831-849
@@ +830,21 @@
+ void setTypeKind(Type *T, StringRef TypeName) {
+ if (TypeName == "sampler_t")
+ TypeKind = (unsigned)KAT_Sampler;
+ else if (TypeName == "queue_t")
+ TypeKind = (unsigned)KAT_Queue;
+ else if (TypeName == "image1d_t" ||
+ TypeName == "image1d_array_t" ||
+ TypeName == "image1d_buffer_t" ||
+ TypeName == "image2d_t" ||
+ TypeName == "image2d_array_t" ||
+ TypeName == "image2d_depth_t" ||
+ TypeName == "image2d_array_depth_t" ||
+ TypeName == "image2d_msaa_t" ||
+ TypeName == "image2d_array_msaa_t" ||
+ TypeName == "image2d_msaa_depth_t" ||
+ TypeName == "image2d_array_msaa_depth_t" ||
+ TypeName == "image3d_t")
+ TypeKind = (unsigned)KAT_Image;
+ else if (isa<PointerType>(T))
+ TypeKind = (unsigned)KAT_Pointer;
+ else
----------------
StringSwitch
================
Comment at: lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:888-897
@@ +887,12 @@
+ for (auto &I:SplitQ) {
+ if (I == "volatile")
+ TypeQual |= (unsigned)KTQ_volatile;
+ else if (I == "restrict")
+ TypeQual |= (unsigned)KTQ_restrict;
+ else if (I == "const")
+ TypeQual |= (unsigned)KTQ_const;
+ else if (I == "pipe")
+ TypeQual |= (unsigned)KTQ_pipe;
+ else
+ llvm_unreachable("invalid type qualifier");
+ }
----------------
You can use a StringSwitch
================
Comment at: lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:901
@@ +900,3 @@
+
+ void setAccessQualifier (StringRef Q) {
+ if (Q == "read_only")
----------------
Extra space before (
================
Comment at: lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:902-911
@@ +901,12 @@
+ void setAccessQualifier (StringRef Q) {
+ if (Q == "read_only")
+ AccQual = KAQ_read_only;
+ else if (Q == "write_only")
+ AccQual = KAQ_write_only;
+ else if (Q == "read_write")
+ AccQual = KAQ_read_write;
+ else if (Q == "none")
+ AccQual = KAQ_none;
+ else
+ llvm_unreachable("invalid access qualifier");
+ }
----------------
Another StringSwitch. Also llvm_unreachable should be avoided
================
Comment at: lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:925-935
@@ +924,13 @@
+ auto T = Arg.getType();
+ auto BaseTypeName = dyn_cast<MDString>(
+ F.getMetadata("kernel_arg_base_type")->getOperand(I))->getString();
+ auto TypeQual = dyn_cast<MDString>(F.getMetadata(
+ "kernel_arg_type_qual")->getOperand(I))->getString();
+ auto AccQual = dyn_cast<MDString>(F.getMetadata(
+ "kernel_arg_access_qual")->getOperand(I))->getString();
+ auto ArgNameMD = F.getMetadata("kernel_arg_name");
+ Flag.setTypeKind(T, BaseTypeName);
+ Flag.setDataType(T, BaseTypeName);
+ Flag.setTypeQualifier(TypeQual);
+ Flag.setAccessQualifier(AccQual);
+ Flag.setAddressQualifier(isa<PointerType>(T) ?
----------------
Can you move this into the Flag constructor and pass in the function?
================
Comment at: lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:936-937
@@ +935,4 @@
+ Flag.setAccessQualifier(AccQual);
+ Flag.setAddressQualifier(isa<PointerType>(T) ?
+ T->getPointerAddressSpace() : 0);
+ Flag.setHasName(ArgNameMD);
----------------
It seems odd to specify 0 as the address pace for no address space since it's private. -1?
================
Comment at: lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:942
@@ +941,3 @@
+ auto DL = F.getParent()->getDataLayout();
+ OutStreamer->EmitIntValue(DL.getTypeSizeInBits(T)/8, 4);
+ OutStreamer->EmitIntValue(DL.getABITypeAlignment(T), 4);
----------------
Use getTypeAllocSize. Make sure there are tests for 3 vectors
================
Comment at: lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:951-952
@@ +950,4 @@
+ if (ArgNameMD) {
+ auto ArgName = dyn_cast<MDString>(ArgNameMD->getOperand(
+ I))->getString();
+ OutStreamer->EmitIntValue(ArgName.size(), 4);
----------------
Unchecked dyn_cast. Use cast or do th enull check
================
Comment at: lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:980-981
@@ +979,4 @@
+ if (RWGS) {
+ OutStreamer->EmitIntValue(mdconst::dyn_extract<ConstantInt>(
+ RWGS->getOperand(0))->getZExtValue(), 4);
+ OutStreamer->EmitIntValue(mdconst::dyn_extract<ConstantInt>(
----------------
Unchecked dyn_extracts
================
Comment at: lib/Target/AMDGPU/AMDGPUAsmPrinter.h:120-122
@@ -119,1 +119,5 @@
+ void EmitStartOfOpenCLMetadata(const Module &M);
+
+ void EmitOpenCLMetadata(const Function &F);
+
----------------
These should start with lowercase e
================
Comment at: test/CodeGen/AMDGPU/kernel-attributes.ll:6
@@ +5,3 @@
+
+; CHECK-LABEL:{{^}}kernel1:
+; CHECK:.section .OpenCL.Metadata
----------------
Spaces after the :s
If you can come up with more descriptive names for the test functions that includes the argument type they test it would be helpful
http://reviews.llvm.org/D21849
More information about the llvm-commits
mailing list