[SM6.10] LinAlg Validation: CopyConvert Builtin#8612
Conversation
alsepkow
left a comment
alsepkow
left a comment
There was a problem hiding this comment.
A few comments from your friendly pair pr reviews @inbelic @Icohedron
| auto DstIt = ValCtx.LinAlgTargetTypeMap.find(DstMatTy); | ||
| auto SrcIt = ValCtx.LinAlgTargetTypeMap.find(SrcMatTy); | ||
| if (DstIt == ValCtx.LinAlgTargetTypeMap.end()) | ||
| break; |
There was a problem hiding this comment.
Am I missing something here, I would have thought these should be an error condition. But the break seems to imply we do nothing?
There was a problem hiding this comment.
My friend copilot pointed out that they are already validated via the ValidateLinAlg* calls at the start of the switch case. So maybe these should be asserts instead?
There was a problem hiding this comment.
They are validated via those calls! We don't want to assert here though as the validation raises validation errors that should be pretty printed and all that jazz. Instead we just stop processing knowing that an error has already been raised and will be reported to the user (along with any other errors seen along the way)
| @@ -0,0 +1,81 @@ | |||
| ; REQUIRES: dxil-1-10 | |||
There was a problem hiding this comment.
Do we plan on adding passing test cases for this later? I noticed that some test files have a filename-passing.ll variant. Looks like all of the existing linalg tests right now are just failing cases?
There was a problem hiding this comment.
We already have passing tests! They are under tools/clang/test/CodeGenDXIL/hlsl/linalg/builtins/<builtin name>/nominal.hlsl
My understanding is that LitDxilValidation is for testing things that should raise validation failures
Co-authored-by: Alex Sepkowski <alexsepkowski@gmail.com>
3 participants
Fixes #8495
Add validation rules for the copy covert builtin