[SM6.10][Bugfix] Add missing 64bit DXIL op overloads#8354
[SM6.10][Bugfix] Add missing 64bit DXIL op overloads#8354V-FEXrt merged 1 commit intomicrosoft:mainfrom
Conversation
V-FEXrt
changed the title
bob80905
left a comment
bob80905
left a comment
There was a problem hiding this comment.
LGTM. Do we need to check for uint64_t or is that redundant?
|
IMO that would be redundant since they both lower to |
| // CHECK2: call void @"dx.hl.op..void (i32, <4 x i64>*, <4 x double>, i32, i32)" | ||
| // CHECK2-SAME: (i32 422, <4 x i64>* %result2, <4 x double> %{{.*}}, i32 1, i32 2) | ||
| double4 vec2 = {9.0, 8.0, 7.0, 6.0}; | ||
| vector<int64_t, 4> result2; |
There was a problem hiding this comment.
Shouldn't we have cases for uint64_t as well, to make sure component type is correctly identified?
There was a problem hiding this comment.
This goes for all tests below too.
There was a problem hiding this comment.
Also, shouldn't we try to match interpretations here?
I guess if all you care about is generating a DXIL op with the overload, this does achieve that, but I would expect validation to come along and make a lot of work for us to update a lot of tests that have these nonsense parameter combinations.
There was a problem hiding this comment.
Matrix component type isn't determined by the types going into the dxil op. That is entirely from the matrix variable
I'm happy to add it if you want but I don't think it adds any meaningful extra coverage
tex3d
left a comment
tex3d
left a comment
There was a problem hiding this comment.
To be clear, my comment shouldn't block the merge, but it does seem odd, and will likely need updates as soon as we start doing validation.
yeah there will be a lot of debt forcibly paid once we start implementing validation |
|
#8355 to track the extra testing needed here! |
3 participants
Fixes #8289
DXIL ops that accept numeric types were supposed to also accept 64 bit wide types. This PR update them.
Changes in
lib/DXIL/DxilOperations.cppare generated