Skip to content

Conversation

@luciechoi
Copy link
Collaborator

@luciechoi luciechoi commented Jan 7, 2026

Part of #7979

  • Add SampledTexture2D resource type in the vk namespace.
  • Add .Sample(float2 location) method.
  • Add default type parameter float4.
  • Allow optional arguments int2 offset, float clamp, uint status.
  • Add .CalculateLevelOfDetail(float2 location) method.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 7, 2026

✅ With the latest revision this PR passed the C/C++ code formatter.

@luciechoi luciechoi marked this pull request as draft January 7, 2026 09:03
Copy link
Collaborator

@s-perron s-perron left a 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.

@luciechoi luciechoi force-pushed the sampledtexture2d branch 3 times, most recently from 67fef39 to 8030b4e Compare January 8, 2026 04:56
@luciechoi luciechoi marked this pull request as ready for review January 8, 2026 05:03
@luciechoi
Copy link
Collaborator Author

Thanks for the early review! I've updated the method to take in optional arguments.

@luciechoi luciechoi requested a review from s-perron January 8, 2026 05:06
@luciechoi luciechoi changed the title [SPIR-V] Add vk::SampledTexture2D resource type and .Sample() method. [SPIR-V] Add vk::SampledTexture2D resource type and .Sample(), CalculateLevelOfDetail() method. Jan 9, 2026
@luciechoi luciechoi changed the title [SPIR-V] Add vk::SampledTexture2D resource type and .Sample(), CalculateLevelOfDetail() method. [SPIR-V] Add vk::SampledTexture2D resource type and .Sample(), .CalculateLevelOfDetail() method. Jan 9, 2026
@damyanp
Copy link
Member

damyanp commented Jan 12, 2026

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.

Copy link
Collaborator

@s-perron s-perron left a 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.

Comment on lines 1402 to 1429
// 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,
Copy link
Collaborator

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, separated them into helpers.

Comment on lines 4016 to 4026
} 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();
Copy link
Collaborator

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I added a check/test to verify the template parameter.

Comment on lines 1374 to 1376
QualType float2Type,
QualType int2Type,
QualType float4Type) {
Copy link
Collaborator

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, generalized the function to take in other SampledTexture types, and pass in the return type and the argument types.

auto loweredType = lowerType(getElementType(astContext, sampledType), rule,
/*isRowMajor*/ llvm::None, srcLoc);

// Treat bool textures as uint for compatibility with OpTypeImage.
Copy link
Collaborator

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the comment.

Comment on lines 5848 to 5856
if (numArgs > 1) {
handleOffsetInMethodCall(expr, 1, &constOffset, &varOffset);
}
if (numArgs > 2) {
clamp = doExpr(expr->getArg(2));
}
if (numArgs > 3) {
status = doExpr(expr->getArg(3));
}
Copy link
Collaborator

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion. I restructured and explicitly stated the indices.

  int samplerIndex, clampIndex, coordIndex, offsetIndex;
  if (isImageSampledTexture) {
    samplerIndex = -1; // non-existant
    coordIndex = 0;
    offsetIndex = 1;
    clampIndex = 2;
  } else {
    samplerIndex = 0;
    coordIndex = 1;
    offsetIndex = 2;
    clampIndex = 3;
  }

This way, I was able to keep some operations in order so the existing tests don't have to be modified.

@s-perron s-perron requested a review from llvm-beanz January 18, 2026 01:11
@s-perron
Copy link
Collaborator

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.

It will not be fully completed for this release. We should mention it in the next.

@luciechoi luciechoi requested a review from s-perron January 22, 2026 22:19
@luciechoi luciechoi force-pushed the sampledtexture2d branch 3 times, most recently from fae5ab0 to aead4cc Compare January 22, 2026 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: New

Development

Successfully merging this pull request may close these issues.

3 participants