-
Notifications
You must be signed in to change notification settings - Fork 16.3k
[RFC][SPIR-V] Add intrinsics to convert to/from ap.float #164252
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
Merged
Merged
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
9119408
[RFC][SPIR-V] Add llvm.arbitrary.fp.convert intrinsic
83ea1ec
remove spirv be part for now
37faca6
Split intrinsic into 2
MrSidims 3492c06
Apply comments
655b487
Merge remote-tracking branch 'origin/main' into mini-fp
241e894
Apply suggestions:
927f12e
format
fdaadcd
Apply suggestions
33c5a89
Merge remote-tracking branch 'origin/main' into mini-fp
a273503
apply comments
55c21e1
apply format
47f7c76
Merge remote-tracking branch 'origin/main' into mini-fp
26ce23d
Remove note, fix comments
38b1fe2
Merge branch 'main' into mini-fp
MrSidims File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add a note to both intrinsics that the supported conversions are target dependent. (These aren't going to get generic legalization support, right?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a note
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should get generic legalization support
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this note just makes things worse. It's stating poor QoI as a goal.
Many intrinsics are broken for different type combinations on different targets, but this isn't a desirable state. There isn't anything target dependent required to legalize these
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My expectation for these intrinsics is that they indeed do not have generic legalization support. They're just a generic spelling for target-specific conversions.
These intrinsics, if they were legalized, should use libcall legalizations, but there are no libcalls for these and I don't expect that they are going to be introduced, so I don't think legalization support makes a lot of sense.
Having someone implement inline expansions for all the type combinations without actually having a use case for it sounds like a massive waste of time.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I had in mind is that for FP4 conversions legalization is quite trivial in case if add a look-up table, then FP4 value becomes just an index. After double checking - FP6 case seem to be also trivial as it's indeed just bit shuffling + rounding.
There are FP8 + FN/FNUZ/E8M0FNU case, where generic lowering stops being “just shuffle + one rounding bit” and becomes “shuffle + full special-case semantics + careful NaN/zero rules.”. I lean towards agreeing, that generic legalization is feasible, yet not 100% sure if there will be interest for the intrinsics outside of the use cases, when a hardware supports the conversions in a performant way.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want me to remove the note and implement generic legalization in this PR? I'm asking as I'm not 100% sure if I'll be able to work on this very PR past January 14th due to a job switch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the note. Lets actually land some implementation and then see how it goes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving the legalization out of this PR is fine for now, but I do think it should be implemented. That significantly increases the utility of the intrinsics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e.g., for single source languages it's very useful to have common operations you can rely on for the host and device code. Restrictions to device-only are limiting