Add asm! support for MIPS #76839

tesuji · 2020-09-17T15:32:50Z

For now, I only add support for mips32.
mips64 may come in future PRs if I could learn more about the target.

rust-highfive · 2020-09-17T15:32:54Z

r? @estebank

(rust_highfive has picked a reviewer for you, use r? to override)

compiler/rustc_target/src/asm/mips.rs

Amanieu · 2020-09-17T16:50:02Z

I think the crash is because LLVM expects register names to be prefixed with $.

compiler/rustc_target/src/asm/mips.rs

Amanieu · 2020-09-17T16:59:59Z

compiler/rustc_target/src/asm/mips.rs

+        _arch: InlineAsmArch,
+    ) -> &'static [(InlineAsmType, Option<&'static str>)] {
+        match self {
+            Self::reg => types! { _: I8, I16, I32; },


For MIPS64 you should add I64. Also most targets support the use of f32 and f64 in general purpose registers for soft-float.

I'm trying for mips32 BE first. Don't know much about MIPS64, but I'd try in other PR.

tesuji · 2020-09-18T02:58:24Z

I think the crash is because LLVM expects register names to be prefixed with $.

Still crashed locally for me after changing register names. GDB Backtrace in https://gist.github.com/lzutao/4f8ae72f9e89e1dfc21ff2dcdd890584 . Looks like some kinds of null deref.

compiler/rustc_target/src/asm/mips.rs

Amanieu · 2020-09-18T04:06:48Z

looks like the crash doesn't happen with llvm-8.

It's just that CI doesn't run these tests since they are marked no-system-llvm.

tesuji · 2020-09-21T04:57:43Z

Codegen fail at this line:
https://github.com/rust-lang/llvm-project/blob/833dd1e3d4fd350c7c9f6fb2ce0c5f16af7a1e21/llvm/include/llvm/CodeGen/TargetLowering.h#L831

Not sure how to resolve this:

rustc: /checkout/src/llvm-project/llvm/include/llvm/CodeGen/TargetLowering.h:831: virtual const llvm::TargetRegisterClass* llvm::TargetLoweringBase::getRegClassFor(llvm::MVT, bool) const: Assertion `RC && "This value type is not natively supported!"' failed.

tesuji · 2020-09-21T11:27:25Z

So here I have more complete backtrack using lldb and release-debuginfo for LLVM:

gdb

warning: used $at (currently $1) without ".set noat"
  --> /home/lzutao/fork/rust/worktree/mips/src/test/assembly/asm/mips-types.rs:52:15
   |
52 |         asm!("move {}, {}", out($class) y, in($class) x);
   |               ^
   |
note: instantiated into assembly here
  --> <inline asm>:1:11
   |
1  |     move $2, $1
   |              ^

rustc: /home/lzutao/fork/rust/worktree/mips/src/llvm-project/llvm/include/llvm/CodeGen/TargetLowering.h:831: virtual const llvm::TargetRegisterClass* llvm::TargetLoweringBase::getRegClassFor(llvm::MVT, bool) const: Assertion `RC && "This value type is not natively supported!"' failed.
Process 5450 stopped
* thread #5, name = 'rustc', stop reason = hit program assert
    frame #4: 0x00007ffff04c26a3 librustc_driver-ff4867624e9288b7.so`llvm::TargetLoweringBase::getRegClassFor(isDivergent=<unavailable>, VT=<unavailable>, this=<unavailable>) const at TargetLowering.h:831:5
   828    virtual const TargetRegisterClass *getRegClassFor(MVT VT, bool isDivergent = false) const {
   829      (void)isDivergent;
   830      const TargetRegisterClass *RC = RegClassForVT[VT.SimpleTy];
-> 831      assert(RC && "This value type is not natively supported!");
   832      return RC;
   833    }
   834

lldb bt

(lldb) bt
* thread #5, name = 'rustc', stop reason = hit program assert
    frame #0: 0x00007fffee87dfff libc.so.6`__GI_raise(sig=6) at raise.c:51
    frame #1: 0x00007fffee87f42a libc.so.6`__GI_abort at abort.c:89
    frame #2: 0x00007fffee876e67 libc.so.6`__assert_fail_base(fmt="", assertion=<unavailable>, file=<unavailable>, line=<unavailable>, function=<unavailable>) at assert.c:92
    frame #3: 0x00007fffee876f12 libc.so.6`__GI___assert_fail(assertion=<unavailable>, file=<unavailable>, line=<unavailable>, function=<unavailable>) at assert.c:101
  * frame #4: 0x00007ffff04c26a3 librustc_driver-ff4867624e9288b7.so`llvm::TargetLoweringBase::getRegClassFor(isDivergent=<unavailable>, VT=<unavailable>, this=<unavailable>) const at TargetLowering.h:831:5
    frame #5: 0x00007ffff04ce8b2 librustc_driver-ff4867624e9288b7.so`llvm::MipsTargetLowering::parseRegForInlineAsmConstraint(llvm::StringRef, llvm::MVT) const at MipsISelLowering.cpp:4099:3
    frame #6: 0x00007ffff04ce8ad librustc_driver-ff4867624e9288b7.so`llvm::MipsTargetLowering::parseRegForInlineAsmConstraint(this=<unavailable>, C=<unavailable>, VT=<unavailable>) const at MipsISelLowering.cpp:4084
    frame #7: 0x00007ffff04e3da0 librustc_driver-ff4867624e9288b7.so`llvm::MipsTargetLowering::getRegForInlineAsmConstraint(this=0x00007fffd80229b0, TRI=0x00007fffd80217c0, Constraint=(Data = "{$8}", Length = 4), VT=(SimpleTy = i8)) const at MipsISelLowering.cpp:4165:54
    frame #8: 0x00007ffff12b0242 librustc_driver-ff4867624e9288b7.so`::GetRegistersForValue(DAG=0x00007fffd8001cb0, DL=0x00007fffecb17260, OpInfo=0x00007fffecb173c0, RefOpInfo=0x00007fffecb173c0)::SDISelAsmOperandInfo &, (anonymous namespace)::SDISelAsmOperandInfo &) at SelectionDAGBuilder.cpp:7954:61
    frame #9: 0x00007ffff12eafe2 librustc_driver-ff4867624e9288b7.so`llvm::SelectionDAGBuilder::visitInlineAsm(this=0x00007fffd8002780, Call=0x00007fffe81c7060) at SelectionDAGBuilder.cpp:8260:25
    frame #10: 0x00007ffff12fc223 librustc_driver-ff4867624e9288b7.so`llvm::SelectionDAGBuilder::visit(this=0x00007fffd8002780, I=0x00007fffe81c7060) at SelectionDAGBuilder.cpp:1141:8
    frame #11: 0x00007ffff135e619 librustc_driver-ff4867624e9288b7.so`llvm::SelectionDAGISel::SelectBasicBlock(this=0x00007fffd80016c0, Begin=<unavailable>, End=llvm::BasicBlock::const_iterator @ rbx, HadTailCall=0x00007fffecb18db0) at SelectionDAGISel.cpp:699:17
    frame #12: 0x00007ffff1362c62 librustc_driver-ff4867624e9288b7.so`llvm::SelectionDAGISel::SelectAllBasicBlocks(this=0x00007fffd80016c0, Fn=0x00007fffe81baa28) at SelectionDAGISel.cpp:1520:27
    frame #13: 0x00007ffff13640d0 librustc_driver-ff4867624e9288b7.so`llvm::SelectionDAGISel::runOnMachineFunction(this=0x00007fffd80016c0, mf=0x00007fffd805c030) at SelectionDAGISel.cpp:504:23
    frame #14: 0x00007ffff04c20fb librustc_driver-ff4867624e9288b7.so`llvm::MipsDAGToDAGISel::runOnMachineFunction(this=0x00007fffd80016c0, MF=0x00007fffd805c030) at MipsISelDAGToDAG.cpp:58:52
    frame #15: 0x00007ffff16735a5 librustc_driver-ff4867624e9288b7.so`llvm::MachineFunctionPass::runOnFunction(this=0x00007fffd80016c0, F=0x00007fffe81baa28) at MachineFunctionPass.cpp:73:33
    frame #16: 0x00007ffff23f1b33 librustc_driver-ff4867624e9288b7.so`llvm::FPPassManager::runOnFunction(this=0x00007fffd8007fb0, F=0x00007fffe81baa28) at LegacyPassManager.cpp:1587:40
    frame #17: 0x00007ffff23f2181 librustc_driver-ff4867624e9288b7.so`llvm::FPPassManager::runOnModule(this=0x00007fffd8007fb0, M=0x00007fffe8101080) at LegacyPassManager.cpp:1629:29
    frame #18: 0x00007ffff23f0ace librustc_driver-ff4867624e9288b7.so`llvm::legacy::PassManagerImpl::run(llvm::Module&) at LegacyPassManager.cpp:1698:38
    frame #19: 0x00007ffff23f0861 librustc_driver-ff4867624e9288b7.so`llvm::legacy::PassManagerImpl::run(this=0x00007fffd8000960, M=0x00007fffe8101080) at LegacyPassManager.cpp:614
    frame #20: 0x00007fffefd9cf55 librustc_driver-ff4867624e9288b7.so`LLVMRustWriteOutputFile + 581
    frame #21: 0x00007fffefd69548 librustc_driver-ff4867624e9288b7.so`rustc_codegen_llvm::back::write::write_output_file::h8f8e8512ef3d7b66 + 88
    frame #22: 0x00007fffefc16907 librustc_driver-ff4867624e9288b7.so`rustc_codegen_llvm::back::write::codegen::with_codegen::h2785a5280d050ea5 + 119
    frame #23: 0x00007fffefd6cc73 librustc_driver-ff4867624e9288b7.so`rustc_codegen_llvm::back::write::codegen::h1dcdb5946b72520a + 2899
    frame #24: 0x00007fffefd0beeb librustc_driver-ff4867624e9288b7.so`rustc_codegen_ssa::back::write::finish_intra_module_work::h63ef7bb6d09628bd + 219
    frame #25: 0x00007fffefd06fa9 librustc_driver-ff4867624e9288b7.so`rustc_codegen_ssa::back::write::execute_work_item::hfe14f77f2358639e + 2745
    frame #26: 0x00007fffefc17ca2 librustc_driver-ff4867624e9288b7.so`std::sys_common::backtrace::__rust_begin_short_backtrace::h7e5ec54f2af1c982 + 194
    frame #27: 0x00007fffefd87c5a librustc_driver-ff4867624e9288b7.so`std::panicking::try::h9dc227032d219e6e + 42
    frame #28: 0x00007fffefc0435d librustc_driver-ff4867624e9288b7.so`core::ops::function::FnOnce::call_once$u7b$$u7b$vtable.shim$u7d$$u7d$::h0fb2f699d71f4606 + 93
    frame #29: 0x00007fffeec56278 libstd-43e93f8b75aa198d.so`_$LT$alloc..boxed..Box$LT$F$GT$$u20$as$u20$core..ops..function..FnOnce$LT$A$GT$$GT$::call_once::h2e60e5e0bf1169eb + 24
    frame #30: 0x00007fffeec6bd1a libstd-43e93f8b75aa198d.so`std::sys::unix::thread::Thread::new::thread_start::h46685771fe1620e2 + 26
    frame #31: 0x00007fffee00f4a4 libpthread.so.0`start_thread(arg=0x00007fffecb1b700) at pthread_create.c:456
    frame #32: 0x00007fffee933d0f libc.so.6`__clone at clone.S:97

tesuji · 2020-09-21T14:45:42Z

I can reproduce the issue. It seems that the generated LLVM-IR missing r modifiers.
Instead of

%0 = call i8 asm sideeffect alignstack "move $$t0, $$t0", "={$8},{$8},~{memory}"(i8 %x), !srcloc !2

it should be

%0 = call i8 asm sideeffect alignstack "move $$t0, $$t0", "=r{$8},r{$8},~{memory}"(i8 %x), !srcloc !2

With r modifiers, the codegen doesn't crash anymore.

LLVM IR and minimal Rust code example that with current state of this PR crashed the codegen: https://rust.godbolt.org/z/c64qG7
llvm_asm! doesn't crash and its llvm-ir contains r modifiers: https://rust.godbolt.org/z/bcrvnG

Amanieu · 2020-09-21T14:48:54Z

Adding the r modifier is wrong: it allows LLVM to use any register instead of just $8. The problem is that LLVM's MIPS backend doesn't support i8 and i16 with inline assembly. Clang has the same issue.

tesuji · 2020-09-21T14:56:39Z

Adding the r modifier is wrong: it allows LLVM to use any register instead of just $8.

Oops, you're right.

Adding the r modifier is wrong: it allows LLVM to use any register instead of just $8. The problem is that LLVM's MIPS backend doesn't support i8 and i16 with inline assembly. Clang has the same issue.

Are there any bugs in bugs.llvm.org/ or this is an intentional design ?
I guess like you said in zulip, we should hack on our side to convert from/to u32.

(Also I currently cannot reply on zulip. The network at night is slow or having issues, I cannot connect to zulip at all).

Amanieu · 2020-09-21T17:19:55Z

We already have a lot of workarounds for LLVM not accepting certain types, we can just add one more.

You'll need to add:

llvm_fixup_input: sign-extend i8 and i16 to i32 (sign-extension is cheaper on MIPS than zero-extension).
llvm_fixup_output: truncate i32 outputs back down to i8 or i16.
llvm_fixup_output_type: use i32 in place of i8 or i16.

You'll also want to add support for f32 (fixup by bitcasting to i32) and i64/f64 for MIPS64.

Finally you should update the asm documentation in src/doc/unstable-book/src/library-features/asm.md.

tesuji · 2020-09-22T09:34:41Z

You'll also want to add support for f32 (fixup by bitcasting to i32) and i64/f64 for MIPS64.

I prefer to do this later, in another PR because I only have a little knowledge for MIPS32 I learned in school.

tesuji · 2020-09-22T11:54:06Z

compiler/rustc_codegen_llvm/src/asm.rs

@@ -706,6 +706,11 @@ fn llvm_fixup_input(
                value
            }
        }
+        (InlineAsmRegClass::Mips(MipsInlineAsmRegClass::reg), Abi::Scalar(s)) => match s.value {
+            Primitive::Int(Integer::I8 | Integer::I16, _) => bx.sext(value, bx.cx.type_i32()),
+            // Primitive::Int(Integer::I8 | Integer::I16, false) => bx.zext(value, bx.cx.type_i32()),


Should we do zero-extend for u8 or u16 ?

No. The asm! documentation specifies that the upper bits of a register are undefined when you pass a smaller value in. So we can do either.

I just ran a few tests on godbolt and it turns out that zero-extension can always be done in 1 instruction while sign-extension sometimes needs 2 (unlike what my previous comment said). You should change this to always zero-extend.

Hmm this is probably confusing since I change my mind halfway through the comment. But basically it doesn't really matter if you sext or zext from the spec point of view.

tesuji · 2020-09-22T12:24:55Z

Finally you should update the asm documentation in src/doc/unstable-book/src/library-features/asm.md.

Which ones you want me to write there? Should I write about LLVM restrictions on i8/i16
and we auto-zero-extend the input arguments to i32 ?

Amanieu · 2020-09-22T12:28:20Z

Just update the various tables with MIPS details. I don't think any other changes are needed.

The existing spec is clear enough for the case of values smaller than a register. We don't want to guarantee zero extension since this may change in the future if LLVM gets proper support for i8/i16.

src/doc/unstable-book/src/library-features/asm.md

Amanieu · 2020-09-26T13:04:23Z

I think you should just use ${{[0-9]+}} for the register class tests like we do for the other architectures. This avoids issues in the future when the register allocator picks a different register.

tesuji · 2020-09-26T13:09:29Z

I want to limit what registers the assembler is able to generate based on general calling convention.
But maybe it is not important here. I'll replace it if you think so.

Amanieu · 2020-09-26T13:10:44Z

Well, what we're really testing here is that the LLVM backend doesn't choke on types it can't support. The actual register number is not important, only that some register is chosen.

Amanieu · 2020-09-26T14:25:54Z

LGTM

This patch also: * Add soft-float supports: only f32 * zero-extend i8/i16 to i32 because MIPS only supports register-length arithmetic. * Update table in asm! chapter in unstable book.

tesuji · 2020-09-27T02:40:19Z

Squashed.

Amanieu · 2020-09-27T02:41:43Z

@bors r+

bors · 2020-09-27T02:41:44Z

📌 Commit 9000710 has been approved by Amanieu

bors · 2020-09-27T07:43:20Z

⌛ Testing commit 9000710 with merge 46d58327c4a259163e2a0f7f4e69b3d74e24bfe6...

bors · 2020-09-27T08:12:24Z

💔 Test failed - checks-actions

tesuji · 2020-09-27T08:13:39Z

curl: (56) SSLRead() return error -9806
clang+llvm-10.0.0-x86_64-apple-darwin/lib/libLLVMX86CodeGen.a: Lzma library error:  No progress is possible
tar: Error exit delayed from previous errors.

from build log: https://github.com/rust-lang-ci/rust/runs/1172123270

kennytm · 2020-09-27T16:07:04Z

@bors retry

…as-schievink Rollup of 7 pull requests Successful merges: - rust-lang#76839 (Add asm! support for MIPS) - rust-lang#77203 (Check for missing const-stability attributes in `rustc_passes`) - rust-lang#77249 (Separate `private_intra_doc_links` and `broken_intra_doc_links` into separate lints) - rust-lang#77252 (reduce overlong line) - rust-lang#77256 (Fix typo in ExpnData documentation) - rust-lang#77262 (Remove duplicate comment) - rust-lang#77263 (Clean up trivial if let) Failed merges: r? `@ghost`

Add asm! support for mips64 - [x] Updated `src/doc/unstable-book/src/library-features/asm.md`. - [ ] No vector type support. I don't know much about those types. cc rust-lang#76839

rust-highfive assigned estebank Sep 17, 2020

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

jyn514 added A-inline-assembly Area: Inline assembly (`asm!(…)`) F-asm `#![feature(asm)]` (not `llvm_asm`) O-MIPS Target: MIPS processors labels Sep 17, 2020

jyn514 reviewed Sep 17, 2020

View reviewed changes

compiler/rustc_target/src/asm/mips.rs Outdated Show resolved Hide resolved

Amanieu reviewed Sep 17, 2020

View reviewed changes

compiler/rustc_target/src/asm/mips.rs Outdated Show resolved Hide resolved

Amanieu reviewed Sep 17, 2020

View reviewed changes

tesuji force-pushed the mips-asm branch from f437f4e to c48d1cc Compare September 18, 2020 02:59

This comment has been minimized.

Sign in to view

Amanieu reviewed Sep 18, 2020

View reviewed changes

compiler/rustc_target/src/asm/mips.rs Outdated Show resolved Hide resolved

tesuji force-pushed the mips-asm branch 2 times, most recently from ead21a2 to 6df5567 Compare September 18, 2020 15:39

tesuji commented Sep 22, 2020

View reviewed changes

Amanieu reviewed Sep 22, 2020

View reviewed changes

src/doc/unstable-book/src/library-features/asm.md Outdated Show resolved Hide resolved

tesuji changed the title ~~[draft, wip] adding asm! support for mips~~ Add asm! support for MIPS Sep 24, 2020

tesuji marked this pull request as ready for review September 24, 2020 09:31

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 26, 2020

Add MIPS asm! support

9000710

This patch also: * Add soft-float supports: only f32 * zero-extend i8/i16 to i32 because MIPS only supports register-length arithmetic. * Update table in asm! chapter in unstable book.

tesuji force-pushed the mips-asm branch from 353f65c to 9000710 Compare September 27, 2020 02:37

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 27, 2020

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

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 27, 2020

This was referenced Sep 27, 2020

Rollup of 8 pull requests #77269

Closed

Rollup of 7 pull requests #77272

Merged

bors merged commit ec1766c into rust-lang:master Sep 27, 2020

rustbot added this to the 1.48.0 milestone Sep 27, 2020

tesuji deleted the mips-asm branch September 27, 2020 23:52

tesuji mentioned this pull request Sep 29, 2020

Add asm! support for mips64 #77337

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add asm! support for MIPS #76839

Add asm! support for MIPS #76839

tesuji commented Sep 17, 2020 •

edited

Loading

rust-highfive commented Sep 17, 2020

Amanieu commented Sep 17, 2020

Amanieu Sep 17, 2020

tesuji Sep 18, 2020

tesuji commented Sep 18, 2020 •

edited

Loading

This comment has been minimized.

Amanieu commented Sep 18, 2020

tesuji commented Sep 21, 2020 •

edited

Loading

tesuji commented Sep 21, 2020 •

edited

Loading

tesuji commented Sep 21, 2020 •

edited

Loading

Amanieu commented Sep 21, 2020

tesuji commented Sep 21, 2020 •

edited

Loading

Amanieu commented Sep 21, 2020

tesuji commented Sep 22, 2020 •

edited

Loading

tesuji Sep 22, 2020

Amanieu Sep 22, 2020

Amanieu Sep 22, 2020

tesuji commented Sep 22, 2020

Amanieu commented Sep 22, 2020

Amanieu commented Sep 26, 2020

tesuji commented Sep 26, 2020 •

edited

Loading

Amanieu commented Sep 26, 2020

Amanieu commented Sep 26, 2020

tesuji commented Sep 27, 2020

Amanieu commented Sep 27, 2020

bors commented Sep 27, 2020

bors commented Sep 27, 2020

bors commented Sep 27, 2020

tesuji commented Sep 27, 2020

kennytm commented Sep 27, 2020

Add asm! support for MIPS #76839

Add asm! support for MIPS #76839

Conversation

tesuji commented Sep 17, 2020 • edited Loading

rust-highfive commented Sep 17, 2020

Amanieu commented Sep 17, 2020

Amanieu Sep 17, 2020

Choose a reason for hiding this comment

tesuji Sep 18, 2020

Choose a reason for hiding this comment

tesuji commented Sep 18, 2020 • edited Loading

This comment has been minimized.

Amanieu commented Sep 18, 2020

tesuji commented Sep 21, 2020 • edited Loading

tesuji commented Sep 21, 2020 • edited Loading

tesuji commented Sep 21, 2020 • edited Loading

Amanieu commented Sep 21, 2020

tesuji commented Sep 21, 2020 • edited Loading

Amanieu commented Sep 21, 2020

tesuji commented Sep 22, 2020 • edited Loading

tesuji Sep 22, 2020

Choose a reason for hiding this comment

Amanieu Sep 22, 2020

Choose a reason for hiding this comment

Amanieu Sep 22, 2020

Choose a reason for hiding this comment

tesuji commented Sep 22, 2020

Amanieu commented Sep 22, 2020

Amanieu commented Sep 26, 2020

tesuji commented Sep 26, 2020 • edited Loading

Amanieu commented Sep 26, 2020

Amanieu commented Sep 26, 2020

tesuji commented Sep 27, 2020

Amanieu commented Sep 27, 2020

bors commented Sep 27, 2020

bors commented Sep 27, 2020

bors commented Sep 27, 2020

tesuji commented Sep 27, 2020

kennytm commented Sep 27, 2020

tesuji commented Sep 17, 2020 •

edited

Loading

tesuji commented Sep 18, 2020 •

edited

Loading

tesuji commented Sep 21, 2020 •

edited

Loading

tesuji commented Sep 21, 2020 •

edited

Loading

tesuji commented Sep 21, 2020 •

edited

Loading

tesuji commented Sep 21, 2020 •

edited

Loading

tesuji commented Sep 22, 2020 •

edited

Loading

tesuji commented Sep 26, 2020 •

edited

Loading