Fix dequantizelinaer simple case with no zero point present #3526
Conversation
Without this we fail in cases when dequantizelinear doesn't have zero point specified
TedThemistokleous
added
bugfix
TedThemistokleous
requested review from
shivadbhavsar,
lakhinderwalia and
richagadgil
and removed request for
lakhinderwalia
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3526 +/- ##
========================================
Coverage 92.17% 92.17%
========================================
Files 512 512
Lines 21385 21386 +1
========================================
+ Hits 19712 19713 +1
Misses 1673 1673 ☔ View full report in Codecov by Sentry. |
| { | ||
| MIGRAPHX_THROW("DEQUANTIZELINEAR: Zero point and input should be the same type."); | ||
| check_shapes{inputs, *this}.same_dims().has(2, 3); |
There was a problem hiding this comment.
Why is this moved down here? We should still check that the inputs are the same dims even when 2 inputs are given. It makes sense up at the top.
There was a problem hiding this comment.
That wont be the case though in this operator would it? I guess we broadcast out scale if its a scalar or its 1-d for the case of scale being a tensor
There was a problem hiding this comment.
That's where the issue was coming from when invoking dequantize_linear, zero points weren't present and resulted in an error.
There was a problem hiding this comment.
What was the error specifically? This allows either 2 or 3 inputs so it shouldn't throw an error for 2 inputs.
There was a problem hiding this comment.
would invoke
terminate called after throwing an instance of 'migraphx::version_1::exception'
what(): /code/AMDMIGraphX/src/include/migraphx/check_shapes.hpp:220: same_dims: dequantizelinear: Dimensions do not match'''
There was a problem hiding this comment.
Unless you want us to always broadcast the scale dimensions (even if its 1-d) but then that means we're inserting a broadcast to remove later. For the scale here, we're never supposed to have the same dimensions between input and scale vectors. Its why I gated this check by input size.
There was a problem hiding this comment.
Actually it looks like the op is intended to work this way where the scales and zero points should already be broadcasted. The parser for dequantizelinear also puts this broadcast in. I dont think this is a bug, your test case should have a broadcast for the scales.
Im not sure what you mean by remove later?
This build is not recommended to merge 🔴 |
🔴bert_large_uncased_fp16: FAILED: MIGraphX is not within tolerance - check verbose output |
|
Not a bug. In MIGraphX we handle this as pointwise so we need to have everything broadcasted to the input to avoid index operations |
Dequantielinear was failing when only two inputs were given (input, scale) as the zero point type check was applied always instead of when there's 3 inputs. Missed due to missing test cases in verify which would have exercised compute_shape() for the op
Reused existing generated onnx files in parser.
Arose when adding changes for #3445 which uses dequantizelinear before the final dot is produced.