[SPIR-V][vk::SampledTexture] #11. Support vk::SampledTexture1D and vk::SampledTexture1DArray type. #8212
[SPIR-V][vk::SampledTexture] #11. Support vk::SampledTexture1D and vk::SampledTexture1DArray type. #8212luciechoi wants to merge 6 commits intomicrosoft:mainfrom
vk::SampledTexture1D and vk::SampledTexture1DArray type. #8212Conversation
luciechoi
force-pushed
the
texture1d
branch
3 times, most recently
from
7d4ff86 to
26674de
Compare
Keenuts
left a comment
Keenuts
left a comment
There was a problem hiding this comment.
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. |
If something is not tested, something can be removed accidentally without us noticing. |
luciechoi
changed the title
vk::SampledTexture1D and vk::SampledTexture1DArray type. vk::SampledTexture1D and vk::SampledTexture1DArray type.
s-perron
left a comment
s-perron
left a comment
There was a problem hiding this comment.
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.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Keenuts
left a comment
Keenuts
left a comment
There was a problem hiding this comment.
Looks overall OK, some code nits, and a more generic remark on tests.
| if (!name.startswith("SampledTexture")) | ||
| return false; | ||
|
|
||
| if (name == "SampledTexture2DMS" || name == "SampledTexture2DMSArray") |
There was a problem hiding this comment.
Since the point here it to allow any SampledTexture, the 2 exact string match just after could be dropped no?
There was a problem hiding this comment.
Here the util is checking whether it's a multi-sampling texture, so it's necessary we add this second check.
| const bool isMS = | ||
| (name == "SampledTexture2DMS" || name == "SampledTexture2DMSArray"); | ||
| // Drop the "SampledTexture" prefix. | ||
| StringRef suffix = name.drop_front(14); |
There was a problem hiding this comment.
Seems like if we do name.drop_front(StringLiteral("Sampledtexture").size()) we get a compilation-time 14 which is self-explanatory.
There was a problem hiding this comment.
Sounds good to get the text size as a compile time constant.
| // CHECK: [[tex_image:%[a-zA-Z0-9_]+]] = OpImage %type_1d_image [[tex1_load]] | ||
| // CHECK: [[fetch_result:%[a-zA-Z0-9_]+]] = OpImageFetch %v4float [[tex_image]] [[pos1]] Lod %uint_0 | ||
| // CHECK: OpStore %a1 [[fetch_result]] | ||
| float4 a1 = tex1d[pos1]; |
There was a problem hiding this comment.
Can we add some amount of negative testing/implicit cast testing?
Make sure we get proper error if the dimension of the passed param is too narow/long, or if the template type of the texture doesn't match the return type?
Make sure we have no gap between what Clang overload resolution accepts and what our SPIR-V lowerering handles?
There was a problem hiding this comment.
Added similar negative test cases from
3 participants
Part of #7979
The function definitions are equivalent to that of Texture1D counterparts, just without Sampler argument.