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

[MIPS] Use softPromoteHalf legalization for fp16 rather than PromoteFloat #110199

Merged
merged 1 commit into from
Nov 5, 2024
Merged

[MIPS] Use softPromoteHalf legalization for fp16 rather than PromoteFloat #110199

merged 1 commit into from
Nov 5, 2024

Conversation

yingopq
Copy link
Contributor

@yingopq yingopq commented Sep 27, 2024 •
edited
Loading

Fix part of #97975.

@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 27, 2024 •
edited
Loading

✅ With the latest revision this PR passed the C/C++ code formatter.

nikic
nikic reviewed Sep 27, 2024
View reviewed changes
Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

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

Fix #97975.

This only fixes it for the MIPS architecture, so should not close the issue with "Fix".

The fix itself here looks fine to me, but I feel like the test coverage is very weak. There's just that one test, and because it's not a generated one, it omits too much information to even tell if the assembly is correct.

llvm/lib/Target/Mips/MipsISelLowering.h Outdated
SDValue Lo = DAG.getNode(MipsISD::Lo, DL, Ty,
getTargetNode(N, Ty, DAG, LoFlag));
SDValue Lo =
DAG.getNode(MipsISD::Lo, DL, Ty, getTargetNode(N, Ty, DAG, LoFlag));
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated reformat? Please use git-clang-format to format PRs, it should leave unrelated lines alone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had used the git clang-format tool before submitting this PR, and it automatically modified the irrelevant code format, so I submitted it together.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would do it locally to check it again.

@yingopq
Copy link
Contributor Author

yingopq commented Sep 27, 2024

Fix #97975.

This only fixes it for the MIPS architecture, so should not close the issue with "Fix".

The fix itself here looks fine to me, but I feel like the test coverage is very weak. There's just that one test, and because it's not a generated one, it omits too much information to even tell if the assembly is correct.

I would update the title and enhance the test file.

@yingopq yingopq changed the title [MIPS] Fix miscompiles consecutive half operations [MIPS] Use softPromoteHalf legalization for fp16 rather than PromoteFloat Sep 30, 2024
nikic
nikic reviewed Oct 1, 2024
View reviewed changes
llvm/test/CodeGen/Mips/fp16-promote.ll Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please rebase over 9f81acf?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nikic Please help review, thanks!

This was referenced Oct 1, 2024
Open
Open
@yingopq
Copy link
Contributor Author

yingopq commented Oct 30, 2024

Ping

@tgross35
Copy link

tgross35 commented Nov 5, 2024 •
edited
Loading

Is it possible to add a test with to_half and from_half from #97981? The codegen be asm-only with no libcalls. Currently __gnu_h2f_ieee / __gnu_f2h_ieee get called https://llvm.godbolt.org/z/M81EsbMbG.

nikic pushed a commit that referenced this pull request Nov 5, 2024
@yingopq @nikic
[Mips] Add additional half float tests (NFC)
a256e89 
For #110199.
@nikic
Copy link
Contributor

nikic commented Nov 5, 2024

@tgross35 Done.

nikic
nikic approved these changes Nov 5, 2024
View reviewed changes
Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

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

LGTM

@nikic nikic merged commit f0231b6 into llvm:main Nov 5, 2024
8 checks passed
PhilippRados pushed a commit to PhilippRados/llvm-project that referenced this pull request Nov 6, 2024
@yingopq @PhilippRados
[Mips] Add additional half float tests (NFC)
f9f4ca0 
For llvm#110199.
PhilippRados pushed a commit to PhilippRados/llvm-project that referenced this pull request Nov 6, 2024
@yingopq @PhilippRados
[MIPS] Use softPromoteHalf legalization for fp16 rather than PromoteF…
bf06e05 
…loat (llvm#110199)

Fix part of llvm#97975.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikic nikic nikic approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yingopq @tgross35 @nikic