Set target-abi module flag for RISC-V targets #123612

kxxt · 2024-04-08T02:15:24Z

Fixes cross-language LTO on RISC-V targets (Fixes #121924)

rustbot · 2024-04-08T02:15:32Z

rustbot has assigned @TaKO8Ki.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

jieyouxu · 2024-04-08T08:46:05Z

Relevant LLVM patch: llvm/llvm-project@227496d
Relevant LLVM issue: llvm/llvm-project#50591

Some remarks (I'm not very familiar with LLVM + RISC-V + LTO):

This will definitetly need a regression test to make sure we don't accidentally break things in the future, I believe your test case could be written as a run-make test for the specific riscv target to make sure it works across LTO, and maybe a codegen(?) test checking that we emit the target-abi metadata for the riscv target.
Can you leave a FIXME for emitting RISC-V-specific target-abi metadata linking to the relevant LLVM issue? AFAICT this is still considered a workaround?
Should we unconditionally emit target-abi metadata?
From what I can tell, if we add target-abi to bitcode, this will trigger an LLVM error/assert if someone specifies an inconsistent link-plugin-opt versus the target-abi (do we want a rustc warning/error here?)

jieyouxu · 2024-04-08T08:52:01Z

Would a current workaround for the reproducer makefile be to specify -Wl,-plugin-opt=-target-abi=lp64d? For rustc it's something like -Clink-args="-target-abi=lp64d"

kxxt · 2024-04-08T10:34:06Z

Hi @jieyouxu, Thanks for your quick review.

Would a current workaround for the reproducer makefile be to specify -Wl,-plugin-opt=-target-abi=lp64d? For rustc it's something like -Clink-args="-target-abi=lp64d"

Yes, that will work, but I still think this should be fixed in rustc.

Should we unconditionally emit target-abi metadata?

I am not familiar with rustc or llvm. But from the code it does look like clang is emitting this metadata unconditionally: https://github.com/llvm/llvm-project/blob/llvmorg-19-init/clang/lib/CodeGen/CodeGenModule.cpp#L1057-L1062

Can you leave a FIXME for emitting RISC-V-specific target-abi metadata linking to the relevant LLVM issue? AFAICT this is still considered a workaround?

I will do it.

This will definitetly need a regression test to make sure we don't accidentally break things in the future, I believe your test case could be written as a run-make test for the specific riscv target to make sure it works across LTO, and maybe a codegen(?) test checking that we emit the target-abi metadata for the riscv target.

I will try to add tests.

jieyouxu · 2024-04-08T10:50:00Z

I personally think that emitting target-abi does make sense. But I'm far from familiar with the LLVM side of things to judge if this approach is what makes the most sense given the context, so I'll leave this to reviewers more well-versed in the LLVM side of things to give the final reviews. I'll tag this PR with A-LLVM for more visibility.

rustbot · 2024-04-08T14:04:26Z

Some changes occurred in run-make tests.

cc @jieyouxu

kxxt · 2024-04-08T14:06:24Z

I have added a run-make test and a codegen test and fixed a bug that empty target-abi got emitted for targets that have llvm_abiname defined as an empty string.

jieyouxu

Thanks for adding the tests. I left some reviews for the tests themselves. For the LLVM wrapper and LLVM codegen changes, another reviewer will look at them.

compiler/rustc_codegen_llvm/src/context.rs

tests/run-make/cross-lang-lto-riscv-abi/Makefile

tests/codegen/riscv-target-abi.rs

tests/run-make/cross-lang-lto-riscv-abi/Makefile

src/tools/tidy/src/allowed_run_make_makefiles.txt

rustbot · 2024-04-09T00:38:19Z

Some changes occurred in src/tools/compiletest

cc @jieyouxu

The run-make-support library was changed

cc @jieyouxu

nikic

The LLVM changes look reasonable to me.

compiler/rustc_codegen_llvm/src/context.rs

tests/codegen/riscv-target-abi.rs

DianQK

I think we can drop cffc0f5 now.

tests/run-make/cross-lang-lto-riscv-abi/rmake.rs

DianQK

I can reproduce the issue when dropping -march. I'm not sure if we also need this parameter. It might not matter.

For me, using drop instead of revert is better. This commit does not contribute to the history.

kxxt · 2024-04-09T03:09:50Z

I can reproduce the issue when dropping -march. I'm not sure if we also need this parameter. It might not matter.

I don't think we should drop -march, because the clang target triple doesn't specify the extensions(especially whether the d ext is available, which should be what causing the default float abi to be different)

For me, using drop instead of revert is better. This commit does not contribute to the history.

Should I keep all the micro commits or squash them?

DianQK · 2024-04-09T03:18:00Z

For me, using drop instead of revert is better. This commit does not contribute to the history.

Should I keep all the micro commits or squash them?

As far as I know, the current rust repository has very liberal requirements for commits. I think it's fine to keep them as long as they are helpful for reviewing the commit history. :)

Fixes cross-language LTO on RISC-V targets (Fixes rust-lang#121924)

jieyouxu · 2024-04-09T19:32:08Z

Thank you for the fix @kxxt! The changes for the intent of this PR looks good to me. I'll make sure to follow it up with the support library work.
@bors r+ rollup

jieyouxu · 2024-04-09T19:33:29Z

wtf is this parsing bors
@bors r-

jieyouxu · 2024-04-09T19:34:01Z

@bors r=jieyouxu,nikic,DianQK rollup

bors · 2024-04-09T19:34:03Z

📌 Commit 33db209 has been approved by jieyouxu,nikic,DianQK

It is now in the queue for this repository.

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#121884 (Port exit-code run-make test to use rust) - rust-lang#122200 (Unconditionally show update nightly hint on ICE) - rust-lang#123568 (Clean up tests/ui by removing `does-nothing.rs`) - rust-lang#123609 (Don't use bytepos offsets when computing semicolon span for removal) - rust-lang#123612 (Set target-abi module flag for RISC-V targets) - rust-lang#123633 (Store all args in the unsupported Command implementation) - rust-lang#123668 (async closure coroutine by move body MirPass refactoring) Failed merges: - rust-lang#123701 (Only assert for child/parent projection compatibility AFTER checking that theyre coming from the same place) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#123612 - kxxt:riscv-target-abi, r=jieyouxu,nikic,DianQK Set target-abi module flag for RISC-V targets Fixes cross-language LTO on RISC-V targets (Fixes rust-lang#121924)

Backport rust-lang/rust#123612 to fix cross-language LTO issues in advance.

run-make: refactor out command wrappers for `clang` and `llvm-readobj` This PR is rebased on top of rust-lang#123699. This PR is a follow up to rust-lang#123612 to refactor out command wrappers into the support library for `llvm-readobj` and `clang`. r? ghost

Rollup merge of rust-lang#123729 - jieyouxu:rmake-refactor-2, r=oli-obk run-make: refactor out command wrappers for `clang` and `llvm-readobj` This PR is rebased on top of rust-lang#123699. This PR is a follow up to rust-lang#123612 to refactor out command wrappers into the support library for `llvm-readobj` and `clang`. r? ghost

rustbot assigned TaKO8Ki Apr 8, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 8, 2024

jieyouxu added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-codegen Area: Code generation labels Apr 8, 2024

kxxt marked this pull request as draft April 8, 2024 10:54

kxxt force-pushed the riscv-target-abi branch from 5556f51 to 85dd0c5 Compare April 8, 2024 13:35

rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Apr 8, 2024

kxxt force-pushed the riscv-target-abi branch from 85dd0c5 to aa6a93a Compare April 8, 2024 14:04

kxxt marked this pull request as ready for review April 8, 2024 14:04

kxxt force-pushed the riscv-target-abi branch from aa6a93a to 66bdd95 Compare April 8, 2024 14:17

jieyouxu requested changes Apr 8, 2024

View reviewed changes

rustbot added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Apr 9, 2024

nikic mentioned this pull request Apr 9, 2024

Add missing target-abi to cross-lang llvm lto bitcode #123400

Closed

nikic reviewed Apr 9, 2024

View reviewed changes

compiler/rustc_codegen_llvm/src/context.rs Outdated Show resolved Hide resolved

tests/codegen/riscv-target-abi.rs Outdated Show resolved Hide resolved

tests/codegen/riscv-target-abi.rs Outdated Show resolved Hide resolved

DianQK reviewed Apr 9, 2024

View reviewed changes

tests/run-make/cross-lang-lto-riscv-abi/rmake.rs Outdated Show resolved Hide resolved

DianQK reviewed Apr 9, 2024

View reviewed changes

tests/run-make/cross-lang-lto-riscv-abi/rmake.rs Show resolved Hide resolved

DianQK approved these changes Apr 9, 2024

View reviewed changes

Set target-abi module flag for RISC-V targets

f19c48e

Fixes cross-language LTO on RISC-V targets (Fixes rust-lang#121924)

This comment was marked as outdated.

Sign in to view

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 9, 2024

This comment was marked as outdated.

Sign in to view

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 9, 2024

This comment was marked as outdated.

Sign in to view

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 9, 2024

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 9, 2024

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 9, 2024

matthiaskrgr mentioned this pull request Apr 10, 2024

Rollup of 7 pull requests #123708

Merged

bors merged commit 2ddf984 into rust-lang:master Apr 10, 2024
11 checks passed

rustbot added this to the 1.79.0 milestone Apr 10, 2024

jieyouxu mentioned this pull request Apr 10, 2024

run-make: refactor out command wrappers for clang and llvm-readobj #123729

Merged

hack3ric added a commit to hack3ric/archriscv-packages that referenced this pull request Apr 18, 2024

updpatch: rust 1:1.77.2-1

f61d372

Backport rust-lang/rust#123612 to fix cross-language LTO issues in advance.

hack3ric mentioned this pull request Apr 18, 2024

updpatch: rust 1:1.77.2-1 felixonmars/archriscv-packages#3727

Merged

felixonmars pushed a commit to felixonmars/archriscv-packages that referenced this pull request Apr 18, 2024

updpatch: rust 1:1.77.2-1

b559422

Backport rust-lang/rust#123612 to fix cross-language LTO issues in advance.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set target-abi module flag for RISC-V targets #123612

Set target-abi module flag for RISC-V targets #123612

kxxt commented Apr 8, 2024

rustbot commented Apr 8, 2024

jieyouxu commented Apr 8, 2024 •

edited

jieyouxu commented Apr 8, 2024 •

edited

kxxt commented Apr 8, 2024

jieyouxu commented Apr 8, 2024 •

edited

rustbot commented Apr 8, 2024

kxxt commented Apr 8, 2024

jieyouxu left a comment

rustbot commented Apr 9, 2024

nikic left a comment

DianQK left a comment

DianQK left a comment •

edited

kxxt commented Apr 9, 2024 •

edited

DianQK commented Apr 9, 2024

jieyouxu commented Apr 9, 2024

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

jieyouxu commented Apr 9, 2024

jieyouxu commented Apr 9, 2024

bors commented Apr 9, 2024

Set target-abi module flag for RISC-V targets #123612

Set target-abi module flag for RISC-V targets #123612

Conversation

kxxt commented Apr 8, 2024

rustbot commented Apr 8, 2024

jieyouxu commented Apr 8, 2024 • edited

jieyouxu commented Apr 8, 2024 • edited

kxxt commented Apr 8, 2024

jieyouxu commented Apr 8, 2024 • edited

rustbot commented Apr 8, 2024

kxxt commented Apr 8, 2024

jieyouxu left a comment

Choose a reason for hiding this comment

rustbot commented Apr 9, 2024

nikic left a comment

Choose a reason for hiding this comment

DianQK left a comment

Choose a reason for hiding this comment

DianQK left a comment • edited

Choose a reason for hiding this comment

kxxt commented Apr 9, 2024 • edited

DianQK commented Apr 9, 2024

jieyouxu commented Apr 9, 2024

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

jieyouxu commented Apr 9, 2024

jieyouxu commented Apr 9, 2024

bors commented Apr 9, 2024

jieyouxu commented Apr 8, 2024 •

edited

jieyouxu commented Apr 8, 2024 •

edited

jieyouxu commented Apr 8, 2024 •

edited

DianQK left a comment •

edited

kxxt commented Apr 9, 2024 •

edited