[SPIRV] Implements struct-to-int casting

[danbrown-amd]

Allows casting a struct to an integer type of at least the same size, which worked for the DXIL target but not previously for SPIR-V.

[s-perron]

This change requires a lot more testing. It is claiming to implement all struct-ti-int casting, but there is only a single test. You will need to test different types of struct: bitfields, floats, vector, structs in structs, small structs, large structs, structs that would have padding, etc..

[s-perron]

How did you determine what the correct behaviour should be? There are lots of cases this should be able to handle, and I'm not sure what they should be doing. For example, what should happen if the struct has multiple fields that are not bitfields? Should they be merged? It seems like the DXIL codes does not merge them: https://godbolt.org/z/1cvc9jq6E. If I am reading your code correctly, It will merge the first two uint16_t, and then assert on the third.

[danbrown-amd]

That is indeed what happens for the test case. I was unable to find any official documentation that specifies the correct behavior; publicly available HLSL documentation is not so detailed, and C and C++ do not allow such casts. If Microsoft has an internal specification, I'd love to see the pertinent section.

I assumed that the result should be 1) the same as accessing fields of a union of the two types if HLSL were extended with C-style unions and 2) the inverse of casting the int type to the struct if the types were the same size. Preliminary examples I tried in DXIL (which I did not preserve, alas) seemed to agree with that interpretation IIRC, but the DXIL for your example doesn't make sense to me: it appears that the cast only applies to the (nested) first field of a struct. Further, that was still the case when I used uint64_t instead of uint. The 16-bit fields shouldn't have 8-byte alignment, should they?

[Keenuts]

reading unoptimized code (-fcgl) the DXIL behavior is a bit clearer IMO:
The first int is zero-extended to match the casted uint size, and copied. Same goes for an uint16_t struct to one uint64_t.
Same story if the struct uses uint32_t, and cast to uint64_t. So seems like the struct layout is ignored, and the cast behaves a bit like indexing the 0 index of an array, and zero-extending to match the output type.

Behavior for bit-fields is the same: it ignores the bitfield layout, seems to take a pointer to the first field of the struct, using the field type as pointer size, and zero-extends it to the casted type?

@llvm-beanz : do we have a proper spec on how this type of cast should be handled?

(code used for the examples above)

struct Color {
  uint16_t r;
  uint16_t g;
  uint16_t b;
};

RWStructuredBuffer<uint> buf : r0;

[numthreads(4, 8, 16)]
void foo() {
  Color s;
  s.r = 4;
  s.g = 5;
  s.b = 6;
  uint64_t value = (uint)s;
}

%9 = getelementptr inbounds i32, i32* %flatconv, i32 0, !dbg !44 ; line:15 col:18
%10 = zext i16 %4 to i32, !dbg !44 ; line:15 col:18
store i32 %10, i32* %9, !dbg !44 ; line:15 col:18

[llvm-beanz]

I wish we had a spec. I've started trying to write a spec for HLSL's implicit conversion sequences (see: microsoft/hlsl-specs#58)

I don't think this should work for bitfields. It does because the interactions between bitfields and HLSL's implicit conversions and initialization sequences are all messed up (see related issue from today #5587). The SPIR-V code generator should likely match whatever DXIL does, and we should fix them both for HLSL 202x.

To dig in way too deep here. I think this should actually produce a warning, because this is an implicit vector truncation (which is allowed, but we warn on). Fixing that isn't easy because this is another prime example of where our ASTs are complete nonsense. The AST dump for this is:

DeclStmt <line:15:3, col:27>
  |   `-VarDecl <col:3, col:26> col:12 value 'uint64_t':'unsigned long long' cinit
  |     `-ImplicitCastExpr <col:20, col:26> 'uint64_t':'unsigned long long' <IntegralCast>
  |       `-CStyleCastExpr <col:20, col:26> 'uint':'unsigned int' <NoOp>
  |         `-ImplicitCastExpr <col:26> 'unsigned int' <FlatConversion>
  |           `-ImplicitCastExpr <col:26> 'Color' <LValueToRValue>
  |             `-DeclRefExpr 0x55b6432e29c0 <col:26> 'Color' lvalue Var 0x55b6432e2520 's' 'Color'

Godbolt link

Notice the complete lack of any HLSLVectorTruncation casts. The cast sequence here just doesn't make sense. It probably should be a FlatConversion from Color to a vector<uint16_t,4>, then a HLSLVectorTruncation to a vector<uint16_t,1>, then an IntegralCast to uint32_t and another IntegralCast to uint64_t.

Fixing the AST representation here is certainly out of scope for what you need in this change. For this change you can probably treat the FlatConversion as an element-wise conversion & optional truncation. That would match what the DXIL generator seems to be doing here.

(cc @tex3d & @pow2clk)

[s-perron]

@danbrown-amd Does this give you enough to move forward with? The important part is:

treat the FlatConversion as an element-wise conversion & optional truncation

In the case of the castToInt, you just need to get the first field. This should simplify the code a little.

[danbrown-amd]

Okay, done.

[s-perron]

The test should include checks to make sure the correct instructions are generated. We should see checks that extract values from c and combine them into a single int. We should also check that the value is stored.

[s-perron]

Sorry for the delay in reviewing. I had for got about it. LGTM. Seems to be matching the DXIL behaviour, so I'm happy.