Skip to content

[RISCV] Fix instruction requires the following: 'D'/'F'/'M' #36461

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

Conversation

trdthg
Copy link
Contributor

@trdthg trdthg commented Nov 10, 2024

@trdthg trdthg requested a review from a team as a code owner November 10, 2024 18:10
@trdthg
Copy link
Contributor Author

trdthg commented Nov 10, 2024

This issue is caused by a bug in llvm, and I think this PR is currently harmless.
Once the bug in llvm is fixed, and the patch is ported to old versions, this change here will no longer be needed

To be honest, I'm not very confident because it has some relationship with llvm. Maybe we should wait for now and check it again after llvm/llvm-project#97685 is merged.

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 10, 2024
@@ -566,6 +566,7 @@ JSValue CLoop::execute(OpcodeID entryOpcodeID, void* executableAddress, VM* vm,
ALIGNMENT \
ALT_ENTRY(label) \
".globl " SYMBOL_STRING(label) "\n" \
".attribute arch, \"rv64gc\"" "\n" \
Copy link
Member

Choose a reason for hiding this comment

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

You need to enable it only for RISCV64.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@trdthg trdthg force-pushed the eng/RISCV-Fix-instruction-requires-the-following-DFM branch from e7ca076 to 97eb199 Compare November 10, 2024 19:01
@trdthg trdthg force-pushed the eng/RISCV-Fix-instruction-requires-the-following-DFM branch from 97eb199 to 6ae82b4 Compare November 10, 2024 19:27
@Constellation
Copy link
Member

Seems like EWS are failing.

@trdthg trdthg force-pushed the eng/RISCV-Fix-instruction-requires-the-following-DFM branch from 6ae82b4 to 57d1228 Compare November 11, 2024 00:51
@trdthg
Copy link
Contributor Author

trdthg commented Nov 11, 2024

done #if -> #elif

@Constellation Constellation added unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing and removed merging-blocked Applied to prevent a change from being merged labels Nov 19, 2024
https://bugs.webkit.org/show_bug.cgi?id=282900

Reviewed by Yusuke Suzuki.

This is a llvm bug, passing `-march=riscv64gc -cpu=lp64d` from clang
will lost after it got llvm side. So we have to set arch here.

More related infomations are here:

- rust-lang/rust#80608
- llvm/llvm-project#61991
- llvm/llvm-project#97685

* Source/JavaScriptCore/assembler/MacroAssemblerRISCV64.cpp:
* Source/JavaScriptCore/llint/LowLevelInterpreter.cpp:

Canonical link: https://commits.webkit.org/286815@main
@webkit-commit-queue webkit-commit-queue changed the title [RISCV] Fix instruction requires the following: 'D'|'F'|'M' [RISCV] Fix instruction requires the following: 'D'/'F'/'M' Nov 19, 2024
@webkit-commit-queue webkit-commit-queue force-pushed the eng/RISCV-Fix-instruction-requires-the-following-DFM branch from 57d1228 to fff4bd8 Compare November 19, 2024 20:26
@webkit-commit-queue
Copy link
Collaborator

Committed 286815@main (fff4bd8): https://commits.webkit.org/286815@main

Reviewed commits have been landed. Closing PR #36461 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit fff4bd8 into WebKit:main Nov 19, 2024
@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Nov 19, 2024
@aperezdc
Copy link
Contributor

Backported in the 2.46 branch as commit 53c63d7

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.

7 participants