Prefer asm! over llvm_asm! in core by tesuji · Pull Request #76669 · rust-lang/rust

tesuji · 2020-09-13T13:01:48Z

Replace llvm_asm! with asm! in core.

x86 asm compare (in somecases I replaced generic type with String).

tesuji · 2020-09-13T13:48:13Z

on llvm8 builder,

 LLVM ERROR: Bad $ operand number in inline asm string: '/* ${0:q} */'

Amanieu · 2020-09-13T13:49:56Z

I think that this is because the new asm! requires LLVM 10. Older LLVM don't support operand modifiers in intel syntax.

tesuji · 2020-09-13T13:51:53Z

I guess I should close this PR and create an issue to track updating to asm! after LLVM 10 ?

Amanieu · 2020-09-13T13:53:08Z

Yes, I think that would be the best approach for now.

tesuji · 2020-09-13T15:00:21Z

Yes, I think that would be the best approach for now.

I added a FIXME in core::hint::black_box instead.

Amanieu · 2020-09-13T15:01:51Z

The same issue applies to the other asm!. You're not getting a CI error because CI only tests x86_64 and not x86.

library/core/src/num/dec2flt/algorithm.rs

tesuji · 2020-09-14T04:19:08Z

I think that this is because the new asm! requires LLVM 10. Older LLVM don't support operand modifiers in intel syntax.

If so, do you think would it be better to document it elsewhere?
And a check for LLVM version to error out sooner ?
Does at&t syntax have the same problem ? If not, is switching to ATT syntax acceptable ?

Amanieu · 2020-09-14T06:40:20Z

I double-checked the LLVM patches, I think that as long as you stick to AT&T syntax and stick to general-purpose registers you should be fine. Obviously this hasn't been tested since the asm! tests are disabled when compiled with system LLVM.

library/core/src/num/dec2flt/algorithm.rs

library/core/src/hint.rs

library/core/src/num/dec2flt/algorithm.rs

Amanieu · 2020-09-14T06:45:07Z

library/core/src/hint.rs

NVPTX is a 64-bit architecture so pointers need to be in reg64, not reg32.

Is this table correct ?

Yes the table is correct. However r is only valid for use with pointers on 32-bit NVPTX. Our NVPTX target is 64-bit so you need to use l for pointers.

Should we document NVPTX64 then ?

library/core/src/hint.rs

Amanieu · 2020-09-15T07:57:52Z

@bors r+

bors · 2020-09-15T07:57:53Z

📌 Commit 1c7204f has been approved by Amanieu

Prefer asm! over llvm_asm! in core Replace llvm_asm! with asm! in core. x86 asm compare (in somecases I replaced generic type with String). * https://rust.godbolt.org/z/59eEMv * https://rust.godbolt.org/z/v78s6q * https://rust.godbolt.org/z/7qYY41

jyn514 · 2020-09-16T04:11:18Z

Failed in rollup (#76774 (comment)):

error[E0472]: asm! is unsupported on this target
   --> library/core/src/hint.rs:143:13
    |
143 | /             asm!(
144 | |                 "/* {0} */",
145 | |                 in(reg) &mut dummy,
146 | |                 options(nostack, preserves_flags),
147 | |             );
    | |______________^

error: aborting due to previous error

[RUSTC-TIMING] core test:false 5.850
error: could not compile `core`.

To learn more, run the command again with --verbose.
command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "mipsel-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "16" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace compiler-builtins-c" "--manifest-path" "/checkout/library/test/Cargo.toml" "--message-format" "json-render-diagnostics"
expected success, got: exit code: 101
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap dist --host mipsel-unknown-linux-gnu --target mipsel-unknown-linux-gnu

jyn514 · 2020-09-16T04:11:41Z

@bors rollup=iffy

tesuji · 2020-09-16T04:51:39Z

I removed the black_box changes and added a FIXME there.

Amanieu · 2020-09-16T09:06:31Z

library/core/src/hint.rs

    unsafe {
-        llvm_asm!("" : : "r"(&dummy));
+        // FIXME: Cannot use `asm!` because it doesn't support MIPS and other architectures.
+        llvm_asm!("" : : "r"(&mut dummy));


Incidentally the old asm had bugs: you need volatile and memory clobbers to have the proper black_box effect. Currently rustc automatically adds volatile and LLVM adds memory when volatile is set, but we probably shouldn't rely on this.

Co-authored-by: Amanieu <amanieu@gmail.com>

Amanieu · 2020-09-16T09:29:13Z

@bors r+

bors · 2020-09-16T09:29:14Z

📌 Commit 4dc4e9f has been approved by Amanieu

Rollup of 10 pull requests Successful merges: - rust-lang#76669 (Prefer asm! over llvm_asm! in core) - rust-lang#76675 (Small improvements to asm documentation) - rust-lang#76681 (remove orphaned files) - rust-lang#76694 (Introduce a PartitioningCx struct) - rust-lang#76695 (fix syntax error in suggesting generic constraint in trait parameter) - rust-lang#76699 (improve const infer error) - rust-lang#76707 (Simplify iter flatten struct doc) - rust-lang#76710 (:arrow_up: rust-analyzer) - rust-lang#76714 (Small docs improvements) - rust-lang#76717 (Fix generating rustc docs with non-default lib directory.) Failed merges: r? `@ghost`

@ghost

Prefer asm! in std - all in sgx module Similar to the change in rust-lang#76669 but all `llvm_asm!` is gate in x86/x86_64 target. Godbolt: - https://rust.godbolt.org/z/h7nG1h - https://rust.godbolt.org/z/xx39hW r? @ghost

Prefer asm! in std - all in sgx module Similar to the change in rust-lang#76669 but all `llvm_asm!` is gate in x86/x86_64 target. Godbolt: - https://rust.godbolt.org/z/h7nG1h - https://rust.godbolt.org/z/xx39hW

rust-highfive assigned Amanieu Sep 13, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 13, 2020

tesuji force-pushed the core_asm branch from 03a4afe to 7dafdb4 Compare September 13, 2020 14:24

tesuji marked this pull request as ready for review September 13, 2020 14:59

Amanieu reviewed Sep 13, 2020

View reviewed changes

library/core/src/num/dec2flt/algorithm.rs Outdated Show resolved Hide resolved

library/core/src/num/dec2flt/algorithm.rs Outdated Show resolved Hide resolved

jyn514 added A-inline-assembly Area: Inline assembly (`asm!(…)`) F-asm `#![feature(asm)]` (not `llvm_asm`) T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Sep 13, 2020

tesuji force-pushed the core_asm branch 2 times, most recently from 6922019 to d5a12d2 Compare September 14, 2020 05:47

Amanieu reviewed Sep 14, 2020

View reviewed changes

library/core/src/num/dec2flt/algorithm.rs Outdated Show resolved Hide resolved

Amanieu reviewed Sep 14, 2020

View reviewed changes

library/core/src/hint.rs Outdated Show resolved Hide resolved

library/core/src/num/dec2flt/algorithm.rs Outdated Show resolved Hide resolved

Amanieu reviewed Sep 14, 2020

View reviewed changes

tesuji mentioned this pull request Sep 14, 2020

Migration from llvm_asm! to asm! rust-lang/stdarch#904

Closed

tesuji force-pushed the core_asm branch from 9739571 to 1c7204f Compare September 15, 2020 05:09

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 Sep 15, 2020

tesuji mentioned this pull request Sep 15, 2020

min-LLVM: Can only use x86 asm ATT syntax for LLVM < 10 in new asm! #76738

Closed

Dylan-DPC-zz mentioned this pull request Sep 16, 2020

Rollup of 26 pull requests #76774

Closed

This comment has been minimized.

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 Sep 16, 2020

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 16, 2020

tesuji force-pushed the core_asm branch from 1c7204f to 627db63 Compare September 16, 2020 04:45

Prefer asm! over llvm_asm! in core

87666e5

tesuji force-pushed the core_asm branch from 627db63 to 87666e5 Compare September 16, 2020 04:48

Amanieu reviewed Sep 16, 2020

View reviewed changes

Fix black_box bug detected by Amanieu

4dc4e9f

Co-authored-by: Amanieu <amanieu@gmail.com>

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 Sep 16, 2020

Dylan-DPC-zz mentioned this pull request Sep 16, 2020

Rollup of 10 pull requests #76786

Merged

bors merged commit d1b0504 into rust-lang:master Sep 16, 2020

rustbot added this to the 1.48.0 milestone Sep 16, 2020

tesuji deleted the core_asm branch September 17, 2020 02:25

tesuji mentioned this pull request Sep 28, 2020

Prefer asm! in std - all in sgx module #77292

Merged

Uh oh!

Conversation

tesuji commented Sep 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tesuji commented Sep 13, 2020

Uh oh!

Amanieu commented Sep 13, 2020

Uh oh!

tesuji commented Sep 13, 2020

Uh oh!

Amanieu commented Sep 13, 2020

Uh oh!

tesuji commented Sep 13, 2020

Uh oh!

Amanieu commented Sep 13, 2020

Uh oh!

Uh oh!

Uh oh!

tesuji commented Sep 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Amanieu commented Sep 14, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Amanieu Sep 14, 2020

Choose a reason for hiding this comment

Uh oh!

tesuji Sep 14, 2020

Choose a reason for hiding this comment

Uh oh!

Amanieu Sep 14, 2020

Choose a reason for hiding this comment

Uh oh!

tesuji Sep 15, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Amanieu commented Sep 15, 2020

Uh oh!

bors commented Sep 15, 2020

Uh oh!

jyn514 commented Sep 16, 2020

Uh oh!

jyn514 commented Sep 16, 2020

Uh oh!

This comment has been minimized.

This comment has been minimized.

tesuji commented Sep 16, 2020

Uh oh!

Amanieu Sep 16, 2020

Choose a reason for hiding this comment

Uh oh!

Amanieu commented Sep 16, 2020

Uh oh!

bors commented Sep 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

tesuji commented Sep 13, 2020 •

edited

Loading

tesuji commented Sep 14, 2020 •

edited

Loading