Add asm clobbers for PPC / PPC64 #111335

Richard-Rogalski · 2023-05-08T04:19:44Z

Marking this as a draft as I have a few questions :D

For the VMX ones, I get error[E0599]: no variant or associated item named 'v19' found for enum 'PowerPCInlineAsmReg' in the current scope. I'm fairly new to rust, and can't quite figure out what's triggering this :). Also, should these be included, when they're not available on every power CPU?

The second thing is this compiles without errors when I make the code specify PowerPC, but not when I make it for PowerPC64: error[E0599]: no variant or associated item named 'PowerPC64' found for enum 'InlineAsmClobberAbi' in the current scope. Some arches (like riscv) appear to use the same for 32bit and 64bit, while others have different clobbers for each. I'm pretty sure the registers correct for both 32 & 64 bit, but will PPC64 use this?

The third thing is r11 and r12. They're both volatile but have 'special' uses.

r11       Volatile register used in calls by pointer and as an
          environment pointer for languages which require one
r12       Volatile register used for exception handling and glink code

I looked at other arches and their registers to see whether I should include these or not, but am unsure :).

@Amanieu

rustbot · 2023-05-08T04:19:51Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @wesleywiser (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

Amanieu · 2023-05-08T05:32:57Z

compiler/rustc_target/src/asm/mod.rs

@@ -885,6 +886,10 @@ impl InlineAsmClobberAbi {
                "C" | "system" | "efiapi" => Ok(InlineAsmClobberAbi::LoongArch),
                _ => Err(&["C", "system", "efiapi"]),
            },
+            InlineAsmArch::PowerPC => match name {


You need to add PowerPC64 here as well if both use the same calling convention. If the registers are different then you need a separate 32-bit and 64-bit InlineAsmClobberAbi.

Amanieu · 2023-05-08T05:39:42Z

For the VMX ones, I get error[E0599]: no variant or associated item named 'v19' found for enum 'PowerPCInlineAsmReg' in the current scope. I'm fairly new to rust, and can't quite figure out what's triggering this :). Also, should these be included, when they're not available on every power CPU?

You need to define the VMX registers in compiler/rustc_target/src/asm/powerpc.rs. You'll need to create a vreg class for them.

The second thing is this compiles without errors when I make the code specify PowerPC, but not when I make it for PowerPC64: error[E0599]: no variant or associated item named 'PowerPC64' found for enum 'InlineAsmClobberAbi' in the current scope. Some arches (like riscv) appear to use the same for 32bit and 64bit, while others have different clobbers for each. I'm pretty sure the registers correct for both 32 & 64 bit, but will PPC64 use this?

See code comments.

The third thing is r11 and r12. They're both volatile but have 'special' uses.

All volatile registers need to be included. The point of clobber_abi is that it should include all registers that may be modified by a function call. This also needs to include the condition code registers.

Richard-Rogalski · 2023-05-12T00:59:43Z

Sorry, forgot pushing updates the PR. Still not done yet :)

Amanieu · 2023-05-12T08:25:47Z

compiler/rustc_target/src/asm/mod.rs

@@ -885,6 +886,10 @@ impl InlineAsmClobberAbi {
                "C" | "system" | "efiapi" => Ok(InlineAsmClobberAbi::LoongArch),
                _ => Err(&["C", "system", "efiapi"]),
            },
+            InlineAsmArch::PowerPC | InlineAsmArch::PowerPC64 => match name {
+                "C" | "system" | "efiapi" => Ok(InlineAsmClobberAbi::PowerPC),


This should not include efiapi.

Amanieu · 2023-05-12T08:30:50Z

compiler/rustc_codegen_llvm/src/asm.rs

@@ -817,6 +818,12 @@ fn dummy_output_type<'ll>(cx: &CodegenCx<'ll, '_>, reg: InlineAsmRegClass) -> &'
        InlineAsmRegClass::PowerPC(PowerPCInlineAsmRegClass::reg) => cx.type_i32(),
        InlineAsmRegClass::PowerPC(PowerPCInlineAsmRegClass::reg_nonzero) => cx.type_i32(),
        InlineAsmRegClass::PowerPC(PowerPCInlineAsmRegClass::freg) => cx.type_f64(),
+        InlineAsmRegClass::PowerPC(PowerPCInlineAsmRegClass::vreg) => {
+            cx.type_vector(cx.type_i64(), 2)
+        } //no clue if this is right :D


This is correct if the tests accept it! You'll need to extend the tests in tests/assembly/asm to include tests for vector regisers. Have a look at the AArch64/x86 tests for examples.

Amanieu · 2023-05-12T08:34:03Z

compiler/rustc_codegen_gcc/src/asm.rs

@@ -610,7 +610,8 @@ fn reg_to_gcc(reg: InlineAsmRegOrRegClass) -> ConstraintOrRegister {
            InlineAsmRegClass::PowerPC(PowerPCInlineAsmRegClass::reg) => "r",
            InlineAsmRegClass::PowerPC(PowerPCInlineAsmRegClass::reg_nonzero) => "b",
            InlineAsmRegClass::PowerPC(PowerPCInlineAsmRegClass::freg) => "f",
-            InlineAsmRegClass::PowerPC(PowerPCInlineAsmRegClass::cr)
+            InlineAsmRegClass::PowerPC(PowerPCInlineAsmRegClass::vreg) => "v",
+            InlineAsmRegClass::PowerPC(PowerPCInlineAsmRegClass::cr)//should this not be either x or y ?


We are explicitly making these clobber-only for now. We can always revisit this later if needed.

Amanieu · 2023-05-12T09:50:24Z

compiler/rustc_target/src/asm/powerpc.rs

@@ -47,6 +48,7 @@ impl PowerPCInlineAsmRegClass {
                }
            }
            Self::freg => types! { _: F32, F64; },
+            Self::vreg => todo!(), //not quite sure :)


I think this should be something like:

types! { vsx: I8, I16, I32, I64, F32, F64, altivec: VecI8(16), VecI16(8), VecI32(4), VecI64(2), VecF32(4), VecF64(2); }

Altivec allows 128-bit vector types, and VSX allows using scalars in vector registers.

However this should be verified by writing asm tests that checks that LLVM is able to generate the correct code for these.

Richard-Rogalski · 2023-05-12T20:37:17Z

Should I make these tests in tests/assembly, or tests/codegen? and should I make a new file for these, or what .rs file would be most appropriate for these kinds of tests?

Amanieu · 2023-05-13T19:25:44Z

The tests should be added to tests/assembly/asm/powerpc-types.rs. You need to test every supported input type with the vreg class, and with an explicit vN register.

Dylan-DPC · 2023-08-12T10:29:19Z

@Richard-Rogalski any updates on this?

Richard-Rogalski · 2023-08-12T19:57:05Z

bah, yeah, I should get back to this. I got a little lost, I'll try again when i get the time

Dylan-DPC · 2024-02-09T07:24:00Z

Closing this as inactive. Feel free to reöpen this pr or create a new pr if you get the time to work on this. Thanks

ky438 · 2024-05-26T08:54:25Z

@Richard-Rogalski - any chance you can get this over the line?

Richard-Rogalski · 2024-05-26T14:42:07Z

I've gotten better at rust in the time since I started this PR. If i am recalling correctly, everything is done except for the tests :p I'll take another crack at this starting today @ky438 .

ky438 · 2024-05-27T03:57:55Z

Thank you! :)

…

On Sun, May 26, 2024 at 07:42:29AM -0700, Richard-Rogalski wrote: I've gotten better at rust in the time since I started this PR. If i am recalling correctly, everything is done except for the tests :p I'll take another crack at this starting today @ky438 . -- Reply to this email directly or view it on GitHub: #111335 (comment) You are receiving this because you were mentioned. Message ID: ***@***.***>

add ppc clobbers

682a64d

rustbot assigned wesleywiser May 8, 2023

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 May 8, 2023

Amanieu reviewed May 8, 2023

View reviewed changes

Add VSX, few other things

42171d5

This comment has been minimized.

Sign in to view

Format, fix build failures

802029c

Amanieu reviewed May 12, 2023

View reviewed changes

Dylan-DPC 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 Aug 12, 2023

Dylan-DPC closed this Feb 9, 2024

Dylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 9, 2024

This was referenced Oct 5, 2024

Tracking Issue for asm_experimental_arch #93335

Open

Support clobber_abi and vector registers (clobber-only) in PowerPC inline assembly #131341

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add asm clobbers for PPC / PPC64 #111335

Add asm clobbers for PPC / PPC64 #111335

Richard-Rogalski commented May 8, 2023

rustbot commented May 8, 2023

Amanieu May 8, 2023

Amanieu commented May 8, 2023

This comment has been minimized.

Richard-Rogalski commented May 12, 2023

Amanieu May 12, 2023

Amanieu May 12, 2023

Amanieu May 12, 2023

Amanieu May 12, 2023

Richard-Rogalski commented May 12, 2023

Amanieu commented May 13, 2023

Dylan-DPC commented Aug 12, 2023

Richard-Rogalski commented Aug 12, 2023

Dylan-DPC commented Feb 9, 2024

ky438 commented May 26, 2024

Richard-Rogalski commented May 26, 2024

ky438 commented May 27, 2024 via email

Add asm clobbers for PPC / PPC64 #111335

Add asm clobbers for PPC / PPC64 #111335

Conversation

Richard-Rogalski commented May 8, 2023

rustbot commented May 8, 2023

Amanieu May 8, 2023

Choose a reason for hiding this comment

Amanieu commented May 8, 2023

This comment has been minimized.

Richard-Rogalski commented May 12, 2023

Amanieu May 12, 2023

Choose a reason for hiding this comment

Amanieu May 12, 2023

Choose a reason for hiding this comment

Amanieu May 12, 2023

Choose a reason for hiding this comment

Amanieu May 12, 2023

Choose a reason for hiding this comment

Richard-Rogalski commented May 12, 2023

Amanieu commented May 13, 2023

Dylan-DPC commented Aug 12, 2023

Richard-Rogalski commented Aug 12, 2023

Dylan-DPC commented Feb 9, 2024

ky438 commented May 26, 2024

Richard-Rogalski commented May 26, 2024

ky438 commented May 27, 2024 via email