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

dialects (arith) add fastmath option for float comparisons #3272

Closed
wants to merge 1 commit into from

Conversation

knickish
Copy link
Contributor

@knickish knickish commented Oct 9, 2024

Add an optional fastmath attribute to arith.cmpf and include the attribute while lowering to riscv

Hopefully closes #2725.

Copy link

codecov bot commented Oct 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@b205332). Learn more about missing BASE report.
Report is 51 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3272   +/-   ##
=======================================
  Coverage        ?   89.97%           
=======================================
  Files           ?      444           
  Lines           ?    55633           
  Branches        ?     5357           
=======================================
  Hits            ?    50055           
  Misses          ?     4177           
  Partials        ?     1401           

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

@superlopuh
Copy link
Member

This is great! Could you please split this into three PRs: arith, riscv, and lowering?

@superlopuh
Copy link
Member

The code pretty much looks ready to merge, but it would be good to add some filecheck tests for the custom format for each of the dialect PRs.

@knickish
Copy link
Contributor Author

@superlopuh I will attempt to split it up. Can you explain filecheck tests for the custom format for each of the dialect PRs a bit more though? I'm not sure exactly what that means

@knickish
Copy link
Contributor Author

knickish commented Oct 10, 2024

Alright, Have it split up into #3275, #3276, and #3277. I think I figured out what you meant by the dialect tests, and each of the PRs has an associated filecheck test now.

@superlopuh
Copy link
Member

The split is great, exactly what I meant, thank you

superlopuh pushed a commit that referenced this pull request Oct 15, 2024
…ons (#3276)

`riscv`-only part of #3272, with
a new dialect test added. Thanks for the suggestion of separate tests, I
didn't realize those existed and caught an error in the LiOp
implementation while adding the test cases.
@knickish knickish closed this Oct 23, 2024
superlopuh added a commit that referenced this pull request Oct 23, 2024
`backend`(lowering)-only part of
#3272. Depends on #3275 and
#3276.

This should close #2725 once merged

Co-authored-by: Sasha Lopoukhine <superlopuh@gmail.com>
EdmundGoodman pushed a commit to EdmundGoodman/xdsl that referenced this pull request Dec 6, 2024
…ons (xdslproject#3276)

`riscv`-only part of xdslproject#3272, with
a new dialect test added. Thanks for the suggestion of separate tests, I
didn't realize those existed and caught an error in the LiOp
implementation while adding the test cases.
EdmundGoodman pushed a commit to EdmundGoodman/xdsl that referenced this pull request Dec 6, 2024
`backend`(lowering)-only part of
xdslproject#3272. Depends on xdslproject#3275 and
xdslproject#3276.

This should close xdslproject#2725 once merged

Co-authored-by: Sasha Lopoukhine <superlopuh@gmail.com>
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.

arith: Missing fastmath property on arith.cmpf
2 participants