Conversation
This comment has been minimized.
This comment has been minimized.
|
Some changes occurred in compiler/rustc_codegen_gcc |
|
This seems okay to me, but I'd rather someone more familiar with this part of the compiler give the final signoff. @bors r? |
|
Error: Parsing assign command in comment failed: ...'' | error: specify user to assign to at >| ''... Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #triagebot on Zulip. |
|
@bors r? compiler |
|
@rustbot reroll |
bdb726b to
9db5dca
Compare
|
Removed return type from tests to fix conflict with #149991, which starts disallowing returns in |
|
The change seems Ok, i'd like people with more background to take a look. |
|
That's not me (sorry it took me a while because of holidays). But iirc that could be amanieu? r? @Amanieu |
|
☔ The latest upstream changes (presumably #150866) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@taiki-e, I saw your mention in the asm tracking issue. amdgpu has named registers (>384 of them), so a clobber_abi would be possible. However, there’s currently no stable ABI, so we would need to follow what the LLVM backend does and I think there’s no tools to make sure the Rust and LLVM ABI understanding is in sync, so that would be asking for subtle breakages. Therefore I think it doesn’t make sense to support clobber_abi with amdgpu at the moment. |
This comment has been minimized.
This comment has been minimized.
|
Rebased to fix conflict, no other changes Edit: kdiff3 didn’t like 🦀 when fixing conflicts 😢, fixed now |
This comment has been minimized.
This comment has been minimized.
|
Hey @Amanieu, are you able to take a look at this PR? Your domain expertise would be really valuable in the review process 🙂 |
| } | ||
|
|
||
| // There are too many conflicts to list | ||
| pub fn overlapping_regs(self, mut _cb: impl FnMut(AmdgpuInlineAsmReg)) {} |
There was a problem hiding this comment.
This still needs to be implemented for correctness, otherwise it might be possible to cause an LLVM assert or crash from rust code.
There was a problem hiding this comment.
Ah, I misinterpreted overlapping_regs before as most targets explicitly list all conflicts between all registers and I estimate it would take some work and memory to store those for amdgpu (164 kiB if I did the math correctly).
Now I see it’s actually just the conflicts for a single register, which should be 30 registers at max.
Edit: Oh, wait, probably more as there’s SIMD registers (see my reply on your next comment)
| sgpr, | ||
| vgpr, |
There was a problem hiding this comment.
If my understanding is correct, a single gpr can only hold 32 bits and you need pairs to get 64-bit values.
So really, the register classes here should be more like the nvptx ones, with separate classes for single registers, pairs and half registers since they all support different kinds of types.
I would expect something like:
- vgpr{16,32,64}
- sgpr{16,32,64}
Maybe with 128-bit variants if i128 support is really needed.
There was a problem hiding this comment.
Hm, they can be used as SIMD/vector registers as well (I added support in #149994), up to [32 x i32].
So, the types would be something like vgpr{16,32,64,96,128,160,192,224,256,288,320,352,384,416,448,480,512,544,576,608,640,672,704,736,768,800,832,864,896,928,960,992,1024}. (a few might not be valid, but the LLVM backend is moving in a direction to support more and more of these)
x86 xmm registers seem to support multiple sizes for a single register class, so maybe amdgpu registers can be modeled like them?
It seems like LLVM and Rust are somewhat forgiving with non-matching types?
E.g. I can assign an i32 to a 16-bit register in x86 (compiles fine though obviously the upper 16-bit of y are garbage afterwards):
let x: i32 = 5;
let y: i32;
unsafe {
asm!("mov cx, ax", lateout("cx") y, in("ax") x);
}Similarly, assigning an i64 to a 32-bit amdgpu sgpr seems to compile fine as well
check_reg!(s0_i642 i64 "s0" "s_mov_b32");There was a problem hiding this comment.
It might compile but the generated asm is definitely incorrect, and this may cause LLVM assert failures in the future even if it doesn't today.
The reason xmm supports multiple sizes is because fundamentally xmm0, ymm0 and zmm0 are just different names for the same register. This is not the case for amdgpu since v0 and v[0:1] are very distinct, for example the latter overlaps v1 but the former doesn't. So these really need to be separate register classes.
Add support for inline assembly for the amdgpu backend (the amdgcn-amd-amdhsa target). Add register classes for `vgpr` (vector general purpose register) and `sgpr` (scalar general purpose register). The LLVM backend supports two more classes, `reg`, which is either VGPR or SGPR, up to the compiler to decide. As instructions often rely on a register being either a VGPR or SGPR for the assembly to be valid, reg doesn’t seem that useful (I struggled to write correct tests for it), so I didn’t end up adding it. The fourth register class is AGPRs, which only exist on some hardware versions (not the consumer ones) and they have restricted ways to write and read from them, which makes it hard to write a Rust variable into them. They could be used inside assembly blocks, but I didn’t add them as Rust register class. There is one change affecting general inline assembly code, that is `InlineAsmReg::name()` now returns a `Cow` instead of a `&'static str`. Because amdgpu has many registers, 256 VGPRs plus combinations of 2 or 4 VGPRs, and I didn’t want to list hundreds of static strings, the amdgpu reg stores the register number(s) and a non-static String is generated at runtime for the register name.
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
Thanks for the review! |
Add support for inline assembly for the amdgpu backend (the amdgcn-amd-amdhsa target).
Add register classes for
vgpr(vector general purpose register) andsgpr(scalar general purpose register).The LLVM backend supports two more classes,
reg, which is either VGPR or SGPR, up to the compiler to decide. As instructions often rely on a register being either a VGPR or SGPR for the assembly to be valid, reg doesn’t seem that useful (I struggled to write correct tests for it), so I didn’t end up adding it.The fourth register class is AGPRs, which only exist on some hardware versions (not the consumer ones) and they have restricted ways to write and read from them, which makes it hard to write a Rust variable into them. They could be used inside assembly blocks, but I didn’t add them as Rust register class.
There is one change affecting general inline assembly code, that is
InlineAsmReg::name()now returns aCowinstead of a&'static str. Because amdgpu has many registers, 256 VGPRs plus combinations of 2 or 4 VGPRs, and I didn’t want to list hundreds of static strings, the amdgpu reg stores the register number(s) and a non-static String is generated at runtime for the register name.Tracking issue: #135024