[SM6.10] LinAlg Validation: VectorAccumulateToDescriptor - #8807
[SM6.10] LinAlg Validation: VectorAccumulateToDescriptor#8807Ashley Coleman (V-FEXrt) wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Implements SM 6.10 validation for VectorAccumulateToDescriptor.
Changes:
- Requires an RWByteAddressBuffer handle.
- Enforces positive, constant, 64-byte-multiple alignment.
- Adds DXIL validation tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
lib/DxilValidation/DxilValidation.cpp |
Implements resource and alignment validation. |
tools/clang/test/LitDXILValidation/LinAlgMatrix/linalgmatrix-vectoraccumulatetodescriptor.ll |
Tests validation diagnostics. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| std::optional<uint64_t> Align = | ||
| ValidateConstantIntGetValue(CI, Op.get_align(), ValCtx, "Align", | ||
| "LinAlgVectorAccumulateToDescriptor"); | ||
| if (Align) { |
There was a problem hiding this comment.
Curious, the issue mentions validating Alignment, but the spec doesn't. And its compiler provided.
Do we want to mention that in the spec?
There was a problem hiding this comment.
Probably yeah, the spec does state that its an immarg which covers that part of the validation rule at least
| if (*Align == 0) | ||
| ValCtx.EmitInstrFormatError(CI, ValidationRule::InstrParamMinimumValue, | ||
| {"Align", "0", std::to_string(*Align)}); | ||
| if (*Align % 64 != 0) |
There was a problem hiding this comment.
Proposal doesn't mention it, but would we also want to confirm its a power of 2?
There was a problem hiding this comment.
Does it need to be a power to 2? I don't think there is any reason for that restriction
| %3 = call %dx.types.Handle @dx.op.annotateHandle(i32 216, %dx.types.Handle %1, %dx.types.ResourceProperties { i32 4107, i32 0 }) ; AnnotateHandle(res,props) resource: RWByteAddressBuffer | ||
|
|
||
| ; okay | ||
| call void @dx.op.linAlgVectorAccumulateToDescriptor.v4f32(i32 -2147483617, %dx.types.Handle %3, i32 0, i32 64, <4 x float> <float 9.000000e+00, float 8.000000e+00, float 7.000000e+00, float 6.000000e+00>) ; LinAlgVectorAccumulateToDescriptor(handle,offset,align,vector) |
There was a problem hiding this comment.
Do we need a 'CHECK-NOT: error'
There was a problem hiding this comment.
another thing copilot has been asking for. We probably should have a CHECK-NOT here, at least at the top of the file but I kinda wanted to go back and fix all of those at once instead of just doing it in the few remaining PRs
There was a problem hiding this comment.
LGTUS - just a couple small suggestions in the tests.
Finn Plummer (@inbelic) Deric C. (@Icohedron)
| %4 = call %dx.types.Handle @dx.op.annotateHandle(i32 216, %dx.types.Handle %2, %dx.types.ResourceProperties { i32 11, i32 0 }) ; AnnotateHandle(res,props) resource: ByteAddressBuffer | ||
|
|
||
| ; CHECK: Function: main: error: LinAlgVectorAccumulateToDescriptor requires RWByteAddressBuffer. | ||
| ; CHECK-NEXT: note: at {{.*}} @dx.op.linAlgVectorAccumulateToDescriptor.v4f32 |
There was a problem hiding this comment.
We could add a test case for a UAV that is not a RawBuffer
There was a problem hiding this comment.
I've been very gently ignoring copilot's request for that and I might ask to do so here was well. You can't actually call these builtins from HLSL with anything besides a RWByteAddressBuffer so I have to manually hack these together to raise the diag. Hacking together the UAV non-RawBuffer is a bit more work that I don't think gives that much extra on the test front.
That said I'm willing to do if it you think its important enough
Fixes #8645
Implements VectorAccumulateToDescriptor validation rules