WIP: HLSL: Suggested changes to allow Load to function on typed vectors and also unsigned input

[dankbaker]

Needed to make these changes to allow some or our code to compile. The changes to Sampler are more WIP, as this value isn't used elsewhere yet. The vector dimension return in HLSL is mostly for validation, so in theory it doesn't need to be used at the moment.

The Loads were not being recognized with unsigned ints as inputs, which is the normal use case for Loads on buffers anyway.

[dankbaker]

Some sample code:

Buffer V_Position : register(t0);

struct VSOut
{
float4 fPos : SV_POSITION;
};

// -----------------------------------------------------------------------
VSOut PosShader( uint VertexID : SV_VertexID )
{
    VSOut Out;
    float4 vPos = float4( V_Position.Load( VertexID ).xyz, 1 );
   Out.fPos = vPos;
   return Out;
}

[johnkslang]

Note we can't merge in PRs that break the test suite; the tests have to be updated with the code change.

A higher-level note is that SPIR-V also cannot have sub-vectors for sampling-function returns. This is the same as the list item

• sub-vec4 return types from sampling

On issue #362. It could be that it should be turned into syntactic sugar for doing a swizzle after the operation, rather than changing the type system, but I agree it deserves deeper investigation.

[dankbaker]

I can't really make heads or tails of the failure, the specific shader appears to compile so I'm not sure exactly what's wrong with it. I will try a few other things, the test harness looked liked i need to install some things to get running.

The return type is mostly syntactic sugar, it does come into play in the AST when you see the return type in a unary expression, since then the AST knows that the vector dimension of the return without needing an explicit cast. I think that case should be pretty easy to put in. That is, an expression like,

Buffer<float3> FooBuffer;
...
float DotP = dot(FooBuffer.Load(0), float3(1,1,1));

You wouldn't need to cast the return value to float3 because the compiler knows that this is the return type. If SPIR-V doesn't have this, then we can just scratch the last component though I think it should probably be added at some point in the future.

Likewise, you can put a struct as a return type, but in that case it needs to be all the same component, and less then 4 components. It really ends up being just a cast. That's a rarely used construct for HLSL, however.

[dankbaker]

I guess this PR can be deleted? I can work around the issue on HLSL code size by putting an explicit (int) cast in front of Loads. Not sure why this auto test is failing though. I'm thinking there is some sort of precedent order when both uint and int are present.

[johnkslang]

Feel free to close. But because you'll want one PR per subject, you might want to keep this PR for it's subject and the other PR for the other subject.

(Normal git model is to make a branch per subject git checkout -b <new-subject> and form a PR from that.)

Fixing the tests shouldn't normally be about "fixing" the shaders; they might be intentionally incorrect anyway. It's a regression suite, so it's really about verifying that the differences in the test results are valid differences, caused as intended by the differences in the source code.

[johnkslang]

the test harness looked liked i need to install some things to get running

Yes, see if the readme (displayed on the home page) has sufficient instructions. Can't really be doing this without that.

[ghost]

turned into syntactic sugar for doing a swizzle after the operation,

Nod - that's been my working assumption, unless the above mentioned deeper investigation uncovers something else. IMHO: while in a sense the existing shape conversions might handle cases of mismatching vectorness, it seems better just to get the type right by adding the swizzle. Then there's no danger about situations where shape conversions do not happen. The info to actually do the swizzle will have to be preserved of course.

Edit for typo.

[johnkslang]

I guess this PR can be deleted?

Yes, I think so, and it is accumulating the -T code changes a well, so something else is needed, thanks.