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.

Sign up for GitHub

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

Jump to bottom

dialects: (llvm) Add a bunch of float methods #3781

Open
wants to merge 1 commit into
base: alexarice/default-prop-in-dict
Choose a base branch
from
Open

dialects: (llvm) Add a bunch of float methods #3781

wants to merge 1 commit into from
+175 −41

Conversation

AntonLydike
Copy link
Collaborator

@AntonLydike AntonLydike commented Jan 23, 2025 •
edited
Loading

Adds the following llvm floating point operations:

  • llvm.fadd
  • llvm.fsub
  • llvm.fmul
  • llvm.fdiv
  • llvm.frem
  • llvm.sitofp
  • llvm.fpext

Closes #3780

I also took the liberty to sort the dialect lists alphabetically

@AntonLydike AntonLydike added the dialects Changes on the dialects label Jan 23, 2025
@AntonLydike AntonLydike self-assigned this Jan 23, 2025
@AntonLydike AntonLydike force-pushed the anton/llvm-float-stuff branch from 47272fd to 73d8169 Compare January 23, 2025 11:12
@codecov Codecov
Copy link

codecov bot commented Jan 23, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.25%. Comparing base (e49877b) to head (f256fae).

Additional details and impacted files 
@@                        Coverage Diff                         @@
##           alexarice/default-prop-in-dict    #3781      +/-   ##
==================================================================
- Coverage                           91.26%   91.25%   -0.01%     
==================================================================
  Files                                 461      461              
  Lines                               57521    57561      +40     
  Branches                             5543     5546       +3     
==================================================================
+ Hits                                52495    52528      +33     
- Misses                               3603     3608       +5     
- Partials                             1423     1425       +2

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

alexarice
alexarice reviewed Jan 23, 2025
View reviewed changes
xdsl/dialects/llvm.py Outdated Show resolved Hide resolved
xdsl/dialects/llvm.py Outdated Show resolved Hide resolved
xdsl/dialects/llvm.py Outdated Show resolved Hide resolved
xdsl/dialects/llvm.py Outdated Show resolved Hide resolved
@alexarice
Copy link
Collaborator

Do we have an mlir integration test for llvm ops? If not I feel it would be useful.

@AntonLydike AntonLydike force-pushed the anton/llvm-float-stuff branch 4 times, most recently from 3a9d650 to 7c83cf7 Compare January 23, 2025 11:43
@AntonLydike
Copy link
Collaborator Author

Do we have an mlir integration test for llvm ops? If not I feel it would be useful.

Added!

@AntonLydike AntonLydike force-pushed the anton/llvm-float-stuff branch 2 times, most recently from 55813c5 to bf4562f Compare January 23, 2025 11:44
alexarice
alexarice reviewed Jan 23, 2025
View reviewed changes
xdsl/dialects/llvm.py Outdated Show resolved Hide resolved
xdsl/dialects/llvm.py Outdated
attributes=attrs if attrs is not None else {},
)

def print(self, printer: Printer):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is necessary, the assembly format from before should still work

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

xdsl.utils.exceptions.PyRDLOpDefinitionError: ('Error during the parsing of the assembly format: ', (Span[38:38](text=''), "fastmathFlags properties are missing from the declarative format. If this is intentional, consider using 'ParsePropInAttrDict' IRDL option."))

Copy link
Collaborator

Choose a reason for hiding this comment

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

Have you considered using the 'ParsePropInAttrDict' IRDL option?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Got it working, thanks!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, no it doesnt! Because fastmath=none flags are not supposed to be printed at all!

Copy link
Collaborator

Choose a reason for hiding this comment

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

This sounds like a bug in assembly format then

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmmmm, yeah maybe we could hide attributes if they are the default value

Copy link
Collaborator

Choose a reason for hiding this comment

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

This should have been happening anyway, but I think I must have missed a case somehow

Copy link
Collaborator

Choose a reason for hiding this comment

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

#3783

@AntonLydike AntonLydike force-pushed the anton/llvm-float-stuff branch 4 times, most recently from bbec3c8 to f9e9a99 Compare January 23, 2025 12:06
@AntonLydike AntonLydike force-pushed the anton/llvm-float-stuff branch 2 times, most recently from 117e9eb to d018456 Compare January 23, 2025 12:12
alexarice
alexarice requested changes Jan 23, 2025
View reviewed changes
Copy link
Collaborator

@alexarice alexarice left a comment

Choose a reason for hiding this comment

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

This should be working with assembly format and I'd prefer to get the bugged fixed rather than introducing custom parsing for something that shouldn't need it

@AntonLydike AntonLydike force-pushed the anton/llvm-float-stuff branch from d018456 to f1c4442 Compare January 23, 2025 12:14
@AntonLydike
Copy link
Collaborator Author

This should be working with assembly format and I'd prefer to get the bugged fixed rather than introducing custom parsing for something that shouldn't need it

But it doesn't.

And fixing the assembly format is outside the scope of this PR.

I'm not able to fix the assembly format today.

@AntonLydike AntonLydike force-pushed the anton/llvm-float-stuff branch 3 times, most recently from cb55182 to 8a3c409 Compare January 23, 2025 13:57
@AntonLydike AntonLydike force-pushed the anton/llvm-float-stuff branch from 8a3c409 to 774aee4 Compare January 23, 2025 14:09
@AntonLydike AntonLydike changed the base branch from main to alexarice/default-prop-in-dict January 23, 2025 14:10
@AntonLydike
Copy link
Collaborator Author

Rebased on top of #3783, @alexarice can you okay now?

alexarice
alexarice approved these changes Jan 23, 2025
View reviewed changes
Copy link
Collaborator

@alexarice alexarice left a comment

Choose a reason for hiding this comment

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

Will leave the parent PR up to give people time to review.

xdsl/dialects/llvm.py Outdated Show resolved Hide resolved
xdsl/dialects/llvm.py


@irdl_op_definition
class SIToFPOp(GenericCastOp):
Copy link
Collaborator

Choose a reason for hiding this comment

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

These are currently very unconstrained but this isn't technically wrong

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it kinda is, you can't cast from a float with this. Added a verify check.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The custom verifier is verify_

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Damn, LinkageAttr also uses the wrong verifier then. Just copied from there as I'm never sure which one to use 🙃

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's different for attributes

@AntonLydike AntonLydike force-pushed the anton/llvm-float-stuff branch from 774aee4 to f256fae Compare January 23, 2025 14:28
alexarice
alexarice reviewed Jan 23, 2025
View reviewed changes
Copy link
Collaborator

@alexarice alexarice left a comment

Choose a reason for hiding this comment

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

I'd prefer to either revert to how it was before or check the input and output types with constraints

xdsl/dialects/llvm.py


@irdl_op_definition
class SIToFPOp(GenericCastOp):
Copy link
Collaborator

Choose a reason for hiding this comment

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

The custom verifier is verify_

xdsl/dialects/llvm.py
if not isinstance(self.arg.type, IntegerType):
raise VerifyException("Expected integer type as input")
if not isinstance(self.result.type, AnyFloat):
raise VerifyException("Expected output to be a float")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't really understand why you would check input and output types in a custom verifier

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

because then I can subclass the main conversion thingy class and keep the remaining bits.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The only shared functionality seems to be the assembly format

@alexarice
Copy link
Collaborator

Could you stop resolving discussions please, makes it hard to check what's happened

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexarice alexarice alexarice approved these changes

@superlopuh superlopuh Awaiting requested review from superlopuh

@lfrenot lfrenot Awaiting requested review from lfrenot

Assignees

@AntonLydike AntonLydike

Labels
dialects Changes on the dialects
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement missing ops for translating sympy to llvm
2 participants
@AntonLydike @alexarice