[DirectX] Add static sampler support to root signature #143422
Conversation
inbelic
removed request for
JDevlieghere,
daniel-grumberg,
cyndyishida,
aaupov,
maksfb,
rafaelauler,
ayermolo,
yota9,
paschalis-mpeis,
lanza,
bcardosolopes,
Endilll,
nikic and
a team
| support::endian::write(BOS, S.RegisterSpace, llvm::endianness::little); | ||
| support::endian::write(BOS, S.ShaderVisibility, llvm::endianness::little); | ||
| if (NumSamplers > 0) { | ||
| rewriteOffsetToCurrentByte(BOS, SSO); |
There was a problem hiding this comment.
So when NumSampler == 0 is required to be 0 right? Or is it just ignored? Or do we set it 0 to match DXC?
Just want to double-check that is defined and correct.
There was a problem hiding this comment.
I set it to zero just to make sure we are not jumping into a random file location by mistake. I don't think DXC forces it to be zero. But if you have no ranges, the offset should be ignored.
There was a problem hiding this comment.
FWIW DXC writes the value where samplers would be unconditionally, so there might be some argument to match that just so that we're binary identical.
That said, you are correct that this offset should be ignored when there aren't any samplers, and I think that explicitly zeroing it makes that clearer, so I kind of like it as is.
| return reportError(Ctx, "Invalid value for AddressW"); | ||
|
|
||
| if (std::optional<APFloat> Val = extractMdFloatValue(StaticSamplerNode, 5)) | ||
| Sampler.MipLODBias = Val->convertToFloat(); |
There was a problem hiding this comment.
Can convertToFloat fail? It might be better to have this as part of extract and return nullopt on failure.
You might need to check opStatus or so?
Even if it doesn't fail, imo, it makes sense to invoke convertToFloat in the extract function anyway
There was a problem hiding this comment.
I did some research, I don't think convertToFloat to float, at least, I didn't see any error handling for it specifically. I moved to inside the extract function anyway.
| } | ||
|
|
||
| static bool verifyMipLODBias(float MipLODBias) { | ||
| return MipLODBias >= -16.f && MipLODBias <= 16.f; |
There was a problem hiding this comment.
I get that it should be <= 15.99.f
| support::endian::write(BOS, S.RegisterSpace, llvm::endianness::little); | ||
| support::endian::write(BOS, S.ShaderVisibility, llvm::endianness::little); | ||
| if (NumSamplers > 0) { | ||
| rewriteOffsetToCurrentByte(BOS, SSO); |
There was a problem hiding this comment.
FWIW DXC writes the value where samplers would be unconditionally, so there might be some argument to match that just so that we're binary identical.
That said, you are correct that this offset should be ignored when there aren't any samplers, and I think that explicitly zeroing it makes that clearer, so I kind of like it as is.
| } | ||
|
|
||
| static bool verifyMipLODBias(float MipLODBias) { | ||
| return MipLODBias >= -16.f && MipLODBias <= 15.99f; |
There was a problem hiding this comment.
I guess this matches what DXC's validator does so it's probably "correct", but 15.99 isn't actually representable in float and this literal is subject to rounding.
| if (!verifySamplerFilter(Sampler.Filter)) | ||
| return reportValueError(Ctx, "Filter", Sampler.Filter); | ||
|
|
||
| if (!verifyAddress(Sampler.AddressU)) |
There was a problem hiding this comment.
I'll take a look at the other PR and think about it there, but FWIW we don't expect to be getting here from a user's input per se - I would expect we only really get here from a root signature read from a binary format or from a frontend that's presumably already attempted to validate the input.
Implements static samplers parsing from root signature metadata representation. This is required to support Root Signatures in HLSL.
Closes: #126641