Add support for const operands and options to global_asm! #84107

Amanieu · 2021-04-11T20:00:05Z

On x86, the default syntax is also switched to Intel to match asm!.

Currently global_asm! only supports const operands and the att_syntax option. In the future, sym operands will also be supported. However there is no plan to support any of the other operand types or options since they don't make sense in the context of global_asm!.

r? @nagisa

rust-highfive · 2021-04-11T20:00:07Z

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

compiler/rustc_codegen_cranelift/src/driver/aot.rs

bors · 2021-04-12T03:45:44Z

☔ The latest upstream changes (presumably #84112) made this pull request unmergeable. Please resolve the merge conflicts.

Amanieu · 2021-04-21T14:30:33Z

This is now ready for review.

nagisa · 2021-04-21T14:41:53Z

I do have this on my list and meant to get through this last weekend, but was busy with more pressing business throughout it, so didn't really have an opportunity to spend any time reviewing.

Amanieu · 2021-04-21T14:46:24Z

There's quite a few changes, but a large part of it is making ItemKind::GlobalAsm(asm) and ExprKind::InlineAsm(asm) both contain the same {ast,hir}::InlineAsm type. The main difference between the two is that global_asm! is currently only allowed to hold const (and in the future sym) operands, and only supports the att_syntax option.

If you don't think that is appropriate then I can change ItemKind::GlobalAsm to use its own separate AST/HIR representation.

nagisa

Welp, that took way longer than I expected it would.

See comments inline. No major issues other than the one with the collector.

r=me once comments are acknowledged/resolved.

compiler/rustc_ast/src/ast.rs

nagisa · 2021-04-24T18:41:46Z

compiler/rustc_codegen_ssa/src/common.rs

+            ty::IntTy::I128 => (value as i128).to_string(),
+            ty::IntTy::Isize => unreachable!(),
+        },
+        ty::Float(ty::FloatTy::F32) => f32::from_bits(value as u32).to_string(),


Is it intended that this uses a decimal representation of the float as opposed to writing out a hex value representing the encoding of the float? I don't recall seeing any other way to represent floats in assembly than their hex encoding.

I'm worried mostly that encoding a float here could lead to double-rounding resembling issues – we had some of those in the past where the Rust's formatting algorithm used to produce very slightly different results than those produced by C, but both still "correct". Encoding the bits representation of the float would make me much more comfortable with this.

That said it seems preexisting… so 🤷

I'm actually considering getting rid of float constants and only supporting integers. It would work better with typeck too.

nagisa · 2021-04-24T18:49:12Z

compiler/rustc_codegen_ssa/src/common.rs

+    };
+    let value = scalar.assert_bits(ty_and_layout.size);
+    match ty_and_layout.ty.kind() {
+        ty::Uint(_) => value.to_string(),


Hm, so I have questions about this. Many architectures have separate instructions that sign-extend or zero-extend the constant operands from, say, 12-bits to the register bit width. Do i recall correctly that otherwise the assembler implementations don't particularly care about whether the constant is represented as a wrapped signed integer or a unsigned integer, as far as it is in range of the maximum bit width for the instruction?

This is probably the correct implementation regardless, but it seems like it could result in some weird action at a distance behaviour.

Our job in rustc is to emit the given constant as a string. The actual interpretation of that string is up to the assembler and is not our problem.

The most sensible way of handling this is to format based on the original type of the value, just like format! does.

The problem is exactly in how different pieces of software interpret the same decimal float string. There's very often a difference between different interpretations. Outputting the float as a hexadecimal bit representation avoids this issue entirely and the only party in control of what float ends up being encoded in the binary is Rust. Which sounds like an improvement to me.

compiler/rustc_mir/src/monomorphize/collector.rs

nagisa · 2021-04-24T20:29:29Z

src/test/ui/asm/type-check-2.rs

+global_asm!("{}", const 0);
+global_asm!("{}", const 0i32);
+global_asm!("{}", const 0f32);
+global_asm!("{}", const 0 as *mut u8);


Allowing pointers may be useful for e.g. embedded use-cases where people might want to refer to e.g. peripherals.

There is a separate sym operand type for this which inserts a symbol name. It is not supported in global_asm! yet but will be in the future.

Ah, I meant something different. Like

const GPIOA: *const u32 = 0x8000_4200;

and then users trying to use that pointer as-is. Symbol isn't quite usable here because placing the symbol in the right place would require linker shenanigans people don't generally love all that much.

src/test/ui/asm/bad-arch.stderr

bors · 2021-04-27T07:34:59Z

☔ The latest upstream changes (presumably #77246) made this pull request unmergeable. Please resolve the merge conflicts.

nagisa · 2021-05-01T10:45:37Z

@bors r+ rollup=never but also see the comments. I think those could still be something for a future improvement.

bors · 2021-05-01T10:45:39Z

📌 Commit 59479edc12a54054c6e3a00d4fb80b79fa257bb9 has been approved by nagisa

bors · 2021-05-01T12:20:59Z

⌛ Testing commit 59479edc12a54054c6e3a00d4fb80b79fa257bb9 with merge 4def5812b498a6b2c87604c9f3bd7868bb01018a...

On x86, the default syntax is also switched to Intel to match asm!

Amanieu · 2021-05-13T21:34:36Z

@bors r=nagisa

bors · 2021-05-13T21:34:37Z

📌 Commit d9cf2ce has been approved by nagisa

Amanieu · 2021-05-13T22:10:16Z

@bors r=nagisa

bors · 2021-05-13T22:10:17Z

📌 Commit a7ed6a5 has been approved by nagisa

bors · 2021-05-13T22:17:47Z

⌛ Testing commit a7ed6a5 with merge 17f30e5...

bors · 2021-05-14T00:58:56Z

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing 17f30e5 to master...

pnkfelix · 2021-05-18T15:34:20Z

Performance triage indicates that this PR introduced a 1.4% regression when fully compiling html5ever-opt.

I don't think any performance hit was expected. However, it also seems like this may be just noise.

rust-highfive assigned nagisa Apr 11, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 11, 2021

This comment has been minimized.

Sign in to view

bjorn3 reviewed Apr 11, 2021

View reviewed changes

compiler/rustc_codegen_cranelift/src/driver/aot.rs Outdated Show resolved Hide resolved

Amanieu force-pushed the global_asm2 branch from db341f8 to 2e0a0c4 Compare April 11, 2021 20:23

Amanieu force-pushed the global_asm2 branch 2 times, most recently from 122c1a0 to 0d3ca4d Compare April 13, 2021 17:11

Amanieu mentioned this pull request Apr 14, 2021

Tracking Issue for inline assembly (asm!) #72016

Closed

4 tasks

Amanieu force-pushed the global_asm2 branch from 0d3ca4d to e7b0f32 Compare April 15, 2021 09:44

This comment has been minimized.

Sign in to view

Amanieu force-pushed the global_asm2 branch from e7b0f32 to 4b8b6c3 Compare April 15, 2021 09:51

This comment has been minimized.

Sign in to view

Amanieu force-pushed the global_asm2 branch from f9b928a to 859386f Compare April 21, 2021 15:01

nagisa approved these changes Apr 24, 2021

View reviewed changes

nagisa 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-review Status: Awaiting review from the assignee but also interested parties. labels Apr 24, 2021

Amanieu force-pushed the global_asm2 branch from fe9ac42 to 59479ed Compare April 27, 2021 23:00

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 May 1, 2021

This comment has been minimized.

Sign in to view

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

Amanieu added 6 commits May 13, 2021 22:31

Add support for const operands and options to global_asm!

5918ee4

On x86, the default syntax is also switched to Intel to match asm!

Add tests for global_asm!

5a229e0

Update global_asm! documentation

0df83f8

Clarify error message when both asm! and global_asm! are unsupported

bb6bec1

global_asm! consts do not depend on other items

40d9da4

Update compiler_builtins to 0.1.43

d9cf2ce

Amanieu force-pushed the global_asm2 branch from d940675 to d9cf2ce Compare May 13, 2021 21:34

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 May 13, 2021

This comment has been minimized.

Sign in to view

Fix tests

a7ed6a5

bors added the merged-by-bors This PR was explicitly merged by bors. label May 14, 2021

bors merged commit 17f30e5 into rust-lang:master May 14, 2021

rustbot added this to the 1.54.0 milestone May 14, 2021

This was referenced May 14, 2021

[WIP] Never type experiments #84573

Closed

Update compiler-builtins to 0.1.42 to get fix for outlined atomics #84764

Closed

The never type and diverging type variables #85021

Closed

Check for inline assembly in THIR unsafeck #85259

Merged

stlankes mentioned this pull request May 19, 2021

switch to intel syntax for assembly code hermit-os/kernel#192

Merged

klensy mentioned this pull request Jun 2, 2021

global_asm expands arguments in comments #85944

Closed

asquared31415 mentioned this pull request Feb 14, 2024

ice: asm const cannot be resolved #121099

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for const operands and options to global_asm! #84107

Add support for const operands and options to global_asm! #84107

Amanieu commented Apr 11, 2021 •

edited

Loading

rust-highfive commented Apr 11, 2021

This comment has been minimized.

bors commented Apr 12, 2021

This comment has been minimized.

Amanieu commented Apr 21, 2021

nagisa commented Apr 21, 2021 •

edited

Loading

Amanieu commented Apr 21, 2021

This comment has been minimized.

nagisa left a comment

nagisa Apr 24, 2021

nagisa Apr 24, 2021

Amanieu Apr 26, 2021

nagisa Apr 24, 2021

Amanieu Apr 26, 2021

nagisa May 1, 2021

nagisa Apr 24, 2021

Amanieu Apr 26, 2021

nagisa May 1, 2021

bors commented Apr 27, 2021

nagisa commented May 1, 2021

bors commented May 1, 2021

bors commented May 1, 2021

This comment has been minimized.

Amanieu commented May 13, 2021

bors commented May 13, 2021

This comment has been minimized.

Amanieu commented May 13, 2021

bors commented May 13, 2021

bors commented May 13, 2021

bors commented May 14, 2021

pnkfelix commented May 18, 2021

Add support for const operands and options to global_asm! #84107

Add support for const operands and options to global_asm! #84107

Conversation

Amanieu commented Apr 11, 2021 • edited Loading

rust-highfive commented Apr 11, 2021

This comment has been minimized.

bors commented Apr 12, 2021

This comment has been minimized.

Amanieu commented Apr 21, 2021

nagisa commented Apr 21, 2021 • edited Loading

Amanieu commented Apr 21, 2021

This comment has been minimized.

nagisa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Apr 27, 2021

nagisa commented May 1, 2021

bors commented May 1, 2021

bors commented May 1, 2021

This comment has been minimized.

Amanieu commented May 13, 2021

bors commented May 13, 2021

This comment has been minimized.

Amanieu commented May 13, 2021

bors commented May 13, 2021

bors commented May 13, 2021

bors commented May 14, 2021

pnkfelix commented May 18, 2021

Amanieu commented Apr 11, 2021 •

edited

Loading

nagisa commented Apr 21, 2021 •

edited

Loading