Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support signed and unsigned integer types in migraphx dialect #1692

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

dhernandez0
Copy link
Contributor

@dhernandez0 dhernandez0 commented Oct 31, 2024

In this PR we change MIGraphX dialect to support singed and unsigned int types.
TODO: add more test (quantize) and e2e tests

@dhernandez0 dhernandez0 self-assigned this Oct 31, 2024
@dhernandez0 dhernandez0 changed the title [work in progress] Support signed and unsigned integer types in migraphx dialect Support signed and unsigned integer types in migraphx dialect Oct 31, 2024
TrivialConverter<RsqrtOp, tosa::RsqrtOp>,
TrivialConverter<SigmoidOp, tosa::SigmoidOp>,
TrivialConverter<TanhOp, tosa::TanhOp>, QuantizeLinearConverter,
DeQuantizeLinearConverter, ConvertConverter, NegConverter,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DeQuantizeLinearConverter was duplicated

Copy link
Collaborator

@krzysz00 krzysz00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me and pretty much what I was thinking. I'm glad I was wildly pessimistic about implementation time here

@@ -117,18 +117,14 @@ def MIGraphX_WhereOp :

def MIGraphX_ConvertOp :
MIGraphX_Op<"convert">,
Arguments<(ins AnyMIXRShaped:$inA, UnitAttr:$zeroExtend)>,
Arguments<(ins AnyMIXRShaped:$inA)>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd keep zeroExtend for the sake of backwards compatibilyt and remove it in a followup PR

Copy link

codecov bot commented Oct 31, 2024

Codecov Report

Attention: Patch coverage is 80.12821% with 31 lines in your changes missing coverage. Please review.

Project coverage is 77.66%. Comparing base (5f51701) to head (0d6bb0f).
Report is 3 commits behind head on develop.

Files with missing lines Patch % Lines
...r/lib/Conversion/MIGraphXToTosa/MIGraphXToTosa.cpp 81.66% 15 Missing and 7 partials ⚠️
...irCustomTosaToLinalg/RocmlirCustomTosaToLinalg.cpp 42.85% 6 Missing and 2 partials ⚠️
...ir/lib/Dialect/MIGraphX/Transforms/RealizeInt4.cpp 93.33% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1692      +/-   ##
===========================================
- Coverage    77.76%   77.66%   -0.10%     
===========================================
  Files          100      100              
  Lines        27866    27953      +87     
  Branches      4063     4080      +17     
===========================================
+ Hits         21671    21711      +40     
- Misses        4540     4575      +35     
- Partials      1655     1667      +12     
Flag Coverage Δ
mfma 77.66% <80.12%> (-0.10%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants