-
Notifications
You must be signed in to change notification settings - Fork 827
[SPIR-V] Add vk::SampledTexture2D resource type and .Sample(), .CalculateLevelOfDetail() method.
#8047
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
s-perron
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a reasonable direction. There are some issues that we need to figure out how to handle.
| // CHECK: [[in_var_TEXCOORD:%[a-zA-Z0-9_]+]] = OpVariable %_ptr_Input_v2float Input | ||
| // CHECK: [[out_var_SV_Target:%[a-zA-Z0-9_]+]] = OpVariable %_ptr_Output_v4float Output | ||
|
|
||
| vk::SampledTexture2D<float4> tex : register(t0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you test this with an int so it is different then the next test? That we will know if you accidentally always output a float type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing out, added a test for vk::SampledTexture2D<uint> type.
tools/clang/lib/Sema/SemaHLSL.cpp
Outdated
| QualType float4Type = | ||
| LookupVectorType(HLSLScalarType::HLSLScalarType_float, 4); | ||
| recordDecl = DeclareVkSampledTexture2DType(*m_context, m_vkNSDecl, | ||
| float2Type, float4Type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the result of the sample have to be a 4 element vector? What happens if the template type is a scalar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, if a template type is scalar, the results are extracted. Added a test.
| if (isSampledTexture(imageType)) { | ||
| sampledImage = image; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we assert that the sampler is false? Not necessary, but this function has a complicated interface, and it could be easy to use it incorrectly.
Is there documentation for this function to update?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we assert that the sampler is false?
Added.
Is there documentation for this function to update?
Updated the function documentation.
67fef39 to
8030b4e
Compare
|
Thanks for the early review! I've updated the method to take in optional arguments. |
vk::SampledTexture2D resource type and .Sample() method.vk::SampledTexture2D resource type and .Sample(), CalculateLevelOfDetail() method.
vk::SampledTexture2D resource type and .Sample(), CalculateLevelOfDetail() method.vk::SampledTexture2D resource type and .Sample(), .CalculateLevelOfDetail() method.
6bc83c2 to
eb29b1b
Compare
eb29b1b to
03b6051
Compare
ea6ac39 to
389cd0f
Compare
|
If this is worth mentioning in the release notes, please add something to https://github.com/microsoft/DirectXShaderCompiler/blob/main/docs/ReleaseNotes.md as appropriate. |
s-perron
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good to me. A few minor code style issues to fix up.
| /// If imageType is not a sampled image type, the OpSampledImage* instructions | ||
| /// will be generated. | ||
| /// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is confusing. Can you rephrase?
| /// If imageType is not a sampled image type, the OpSampledImage* instructions | |
| /// will be generated. | |
| /// | |
| /// If the of `image` is a sampled image, then that image will be sampled. In this case, `sampler` must be `nullptr`. If `image` is not a sampled image, a sampled image will be create by combining `image` and `sampler`. | |
| /// |
You'll need to fix the formatting.
| // Sample(location, offset) | ||
| QualType params2[] = {float2Type, int2Type}; | ||
| StringRef names2[] = {"location", "offset"}; | ||
| CXXMethodDecl *sampleDecl2 = CreateObjectFunctionDeclarationWithParams( | ||
| context, recordDecl, paramType, params2, names2, | ||
| context.DeclarationNames.getIdentifier(&context.Idents.get("Sample")), | ||
| /*isConst*/ true); | ||
| sampleDecl2->addAttr(HLSLIntrinsicAttr::CreateImplicit( | ||
| context, "op", "", static_cast<int>(hlsl::IntrinsicOp::MOP_Sample))); | ||
| sampleDecl2->addAttr(HLSLCXXOverloadAttr::CreateImplicit(context)); | ||
|
|
||
| // Sample(location, offset, clamp) | ||
| QualType params3[] = {float2Type, int2Type, floatType}; | ||
| StringRef names3[] = {"location", "offset", "clamp"}; | ||
| CXXMethodDecl *sampleDecl3 = CreateObjectFunctionDeclarationWithParams( | ||
| context, recordDecl, paramType, params3, names3, | ||
| context.DeclarationNames.getIdentifier(&context.Idents.get("Sample")), | ||
| /*isConst*/ true); | ||
| sampleDecl3->addAttr(HLSLIntrinsicAttr::CreateImplicit( | ||
| context, "op", "", static_cast<int>(hlsl::IntrinsicOp::MOP_Sample))); | ||
| sampleDecl3->addAttr(HLSLCXXOverloadAttr::CreateImplicit(context)); | ||
|
|
||
| // Sample(location, offset, clamp, status) | ||
| QualType params4[] = {float2Type, int2Type, floatType, | ||
| context.getLValueReferenceType(uintType)}; | ||
| StringRef names4[] = {"location", "offset", "clamp", "status"}; | ||
| CXXMethodDecl *sampleDecl4 = CreateObjectFunctionDeclarationWithParams( | ||
| context, recordDecl, paramType, params4, names4, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function will be come very big. Could you break it up into smaller functions? This block could be its own. Call it addSampeFunction. You can group other functions in the same way.
| } else if (kind == AR_OBJECT_VK_SAMPLED_TEXTURE2D) { | ||
| if (!m_vkNSDecl) | ||
| continue; | ||
| recordDecl = DeclareVkSampledTexture2DType( | ||
| *m_context, m_vkNSDecl, | ||
| LookupVectorType(HLSLScalarType::HLSLScalarType_float, 2), | ||
| LookupVectorType(HLSLScalarType::HLSLScalarType_int, 2), | ||
| LookupVectorType(HLSLScalarType::HLSLScalarType_float, 4)); | ||
| recordDecl->setImplicit(true); | ||
| m_vkSampledTexture2DTemplateDecl = | ||
| recordDecl->getDescribedClassTemplate(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should add some tests in sema that look for errors. What if an invalid type is given as a template parameter?
| QualType float2Type, | ||
| QualType int2Type, | ||
| QualType float4Type) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you planning on make this function more general? For example could float2Type become CoordinateType? Then for Textur3D it is just a float?
If not, why pass these in as parameters? Could we simplify the interface an build them in this function?
| auto loweredType = lowerType(getElementType(astContext, sampledType), rule, | ||
| /*isRowMajor*/ llvm::None, srcLoc); | ||
|
|
||
| // Treat bool textures as uint for compatibility with OpTypeImage. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not about compatibility with OpTypeImage. Bool does not have a defined size in SPIR-V, so it cannot be used in the external interface.
| auto *objectInfo = loadIfGLValue(imageExpr); | ||
| auto *samplerState = | ||
| isSampledTexture(imageType) ? nullptr : doExpr(expr->getArg(0)); | ||
| auto *coordinate = isSampledTexture(imageType) ? doExpr(expr->getArg(0)) | ||
| : doExpr(expr->getArg(1)); | ||
|
|
||
| auto *sampledImage = | ||
| isSampledTexture(imageType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are checking isSampledTextured a lot in this code. You should just use a single if-statement.
| if (numArgs > 1) { | ||
| handleOffsetInMethodCall(expr, 1, &constOffset, &varOffset); | ||
| } | ||
| if (numArgs > 2) { | ||
| clamp = doExpr(expr->getArg(2)); | ||
| } | ||
| if (numArgs > 3) { | ||
| status = doExpr(expr->getArg(3)); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You eventually going to have to add all of the checks below when you add all of the appropriate overload. We should try to avoid adding duplicating the argument parsing here.
You could try doing:
uint offset = 0;
auto *image = loadIfGLValue(imageExpr);
auto *sampler = nullptr;
if (!isSampledTexture(imageType)) {
sampler = doExpr(expr->getArg(0));
offset = 1;
}
Then all of the later accesses become to expr->getArg(offset + ); For example:
auto *coordinate = doExpr(expr->getArg(offset));
This will avoid repeating the logic.
It will not be fully completed for this release. We should mention it in the next. |
Part of #7979
SampledTexture2Dresource type in thevknamespace..Sample(float2 location)method.float4.int2 offset, float clamp, uint status..CalculateLevelOfDetail(float2 location)method.