-
Notifications
You must be signed in to change notification settings - Fork 15.4k
[RISCV][CostModel] Add cost for fabs/fsqrt of type bf16/f16 #118608
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 5 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
48dfe67
[RISCV][CostModel] Add cost for fabs/fsqrt of type bf16/f16
LiqinWeng e682e00
Remove the condition for sqrts
LiqinWeng 86bd2f7
Promote the bf16/f16 to fp32 to calculate the cost
LiqinWeng 0b5f44e
Merge branch 'main' into fabs-fsqrt-f16-bf16-cost
LiqinWeng 1fe4655
Add cost for fsqrt of type nxv32[b]f16
LiqinWeng b7c046f
update nxv32[b]f16 to nxv16f32
LiqinWeng 3bb8859
Merge branch 'main' into fabs-fsqrt-f16-bf16-cost
LiqinWeng 1a8c67e
fix the coments and clang-format for test
LiqinWeng 5d2432f
fix the vtype for fcvt
LiqinWeng 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
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
Oops, something went wrong.
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.
NVT needs to be changed to v16f32.
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.
fixed
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.
Can you help me see the code check? I feel that there is no problem with the code, but the code check fails.
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.
@LiqinWeng Are you trying to compute the cost for ISAs without f16 support as (vfwcvt, nxvf16) + (vfsqrt, nxvf32) + (vfncvt, nxv*f16)? From the current code, it doesn’t seem like that’s what’s being implemented.
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.
Under what circumstances will it occur: (vfwcvt, nxvf16) + (vfsqrt, nxvf32) + (vfncvt, nxv*f16)???I don't quite understand ‘ISAs without f16’? do you means the mattr only support zvfh???
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.
There's a vfwcvt.f.f.v from EEW=16 EMUL=m1 to EEW=32 EMUL=m2; Shouldn't the type of vfwcvt be nxvf32, the dest eew is 32
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.
@LiqinWeng vfwcvt is a widening arithmetic instruction. Based on the spec description, the destination EEW is
2 * SEW, so the vtype is nxvf16 and the destination is nxvf32.On the other hand, for narrowing arithmetic instruction such as
vfncvt, the destination EEW isSEW, so both the vtype and the destination are nxvf16.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.
Is this the case with this instruction(vfwcvtbf16.f.f.v )?
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.
hmm.. I think vfwcvtbf16.f.f.v is the same with vfwcvt.f.f.v.
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.
Fixed