Skip to content

[SPIR-V][vk::SampledTexture] #11. Support vk::SampledTexture1D and vk::SampledTexture1DArray type. #8212

Merged
luciechoi merged 6 commits intomicrosoft:mainfrom
luciechoi:texture1d
Apr 13, 2026
Merged

[SPIR-V][vk::SampledTexture] #11. Support vk::SampledTexture1D and vk::SampledTexture1DArray type. #8212
luciechoi merged 6 commits intomicrosoft:mainfrom
luciechoi:texture1d

Conversation

@luciechoi
Copy link
Copy Markdown
Collaborator

Part of #7979

The function definitions are equivalent to that of Texture1D counterparts, just without Sampler argument.

@luciechoi
Copy link
Copy Markdown
Collaborator Author

(@Keenuts @s-perron next SampledTexture PR for review)

Copy link
Copy Markdown
Collaborator

@Keenuts Keenuts left a comment

Choose a reason for hiding this comment

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

Overall looks good, but many overloads are not tested. I stopped checking after a few, but overall we should have at least one test for each overload.

Comment thread utils/hct/gen_intrin_main.txt Outdated
Comment thread utils/hct/gen_intrin_main.txt Outdated
Comment thread utils/hct/gen_intrin_main.txt Outdated
Comment thread utils/hct/gen_intrin_main.txt Outdated
Comment thread utils/hct/gen_intrin_main.txt Outdated
Comment thread utils/hct/gen_intrin_main.txt Outdated
Comment thread utils/hct/gen_intrin_main.txt Outdated
@github-project-automation github-project-automation Bot moved this from New to In progress in HLSL Roadmap Mar 19, 2026
@luciechoi
Copy link
Copy Markdown
Collaborator Author

Overall looks good, but many overloads are not tested. I stopped checking after a few, but overall we should have at least one test for each overload.

Hmmm do you mean every function variant should have a test..? The overloads are tested with SampledTexture2D type, and adding a 1D type doesn't require code changes.

I took a look at adding these, but noticed that the existing test coverage for the other texture types (like Texture2D, Texture2DMS, and their array counterparts) also doesn't exhaustively test every single overload.

To keep the scope of this PR manageable and maintain parity with regular Texture types, I've mirrored the existing level of coverage for the 1D types. Let me know if there's a specific overload variant you are concerned about and I'd be happy to add a test just for that one.

@luciechoi luciechoi requested a review from Keenuts March 20, 2026 00:50
@Keenuts
Copy link
Copy Markdown
Collaborator

Keenuts commented Mar 20, 2026

Hmmm do you mean every function variant should have a test..? The overloads are tested with SampledTexture2D type, and adding a 1D type doesn't require code changes.

If something is not tested, something can be removed accidentally without us noticing.
That's why some projects implement mutation testing or similar.
The test does not have to be complex, but we should make sure each overload is at least present through a test.

Copy link
Copy Markdown
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.

I agree that we need to haves testing for all for all of the new functions, even if the code generating them is the same. I think you are working on that.

We can also improve some string checking, which can be slow.

Comment thread tools/clang/lib/SPIRV/LowerTypeVisitor.cpp Outdated
Comment thread tools/clang/lib/SPIRV/LowerTypeVisitor.cpp Outdated
@luciechoi luciechoi changed the title [SPIR-V][vk::SampledTexture] #10. Support vk::SampledTexture1D and vk::SampledTexture1DArray type. [SPIR-V][vk::SampledTexture] #11. Support vk::SampledTexture1D and vk::SampledTexture1DArray type. Mar 26, 2026
@luciechoi
Copy link
Copy Markdown
Collaborator Author

@Keenuts @s-perron

I've added the tests for all function overloads, PTAL!

@luciechoi luciechoi requested a review from s-perron March 26, 2026 03:31
Copy link
Copy Markdown
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.

I think we should try to make the string matching good. I don't think it is hard to read the code if you use startswith for an initial check, drop the prefix, and then check the rest.

Comment thread tools/clang/lib/SPIRV/AstTypeProbe.cpp Outdated
Comment thread tools/clang/lib/SPIRV/LowerTypeVisitor.cpp Outdated
@luciechoi luciechoi requested a review from s-perron March 31, 2026 02:00
@luciechoi
Copy link
Copy Markdown
Collaborator Author

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@luciechoi luciechoi enabled auto-merge (squash) April 1, 2026 08:11
@luciechoi luciechoi disabled auto-merge April 1, 2026 08:11
@luciechoi luciechoi enabled auto-merge (squash) April 1, 2026 08:11
Copy link
Copy Markdown
Collaborator

@Keenuts Keenuts left a comment

Choose a reason for hiding this comment

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

Looks overall OK, some code nits, and a more generic remark on tests.

Comment thread tools/clang/lib/SPIRV/AstTypeProbe.cpp
const bool isMS =
(name == "SampledTexture2DMS" || name == "SampledTexture2DMSArray");
// Drop the "SampledTexture" prefix.
StringRef suffix = name.drop_front(14);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Seems like if we do name.drop_front(StringLiteral("Sampledtexture").size()) we get a compilation-time 14 which is self-explanatory.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good to get the text size as a compile time constant.

@luciechoi luciechoi merged commit 71bf911 into microsoft:main Apr 13, 2026
12 checks passed
@github-project-automation github-project-automation Bot moved this from In progress to Done in HLSL Roadmap Apr 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants