esp-rs / rust

Rust for the xtensa architecture. Built in targets for the ESP32 and ESP8266
https://www.rust-lang.org
Other
744 stars 39 forks source link

Not supported instr error #95

Closed bugadani closed 2 years ago

bugadani commented 3 years ago

I have the following snippet:

pub enum Enum<'a> {
    A(&'a str),
    B { ptr: usize, len: usize },
    C(&'a [u8]),
    D(u8),
}

impl Enum<'_> {
    pub(crate) fn foo(&self, tmp: &mut Vec<u8>) {
        match self {
            Self::A(_) => tmp.push(0),
            Enum::B { .. } => tmp.push(1),
            Enum::C(_) => tmp.push(2),
            Enum::D(_) => tmp.push(3),
        }
    }
}

pub struct B<'a> {
    slice: &'a [Enum<'a>],
}
impl B<'_> {
    pub fn foo(&self, tmp: &mut Vec<u8>) {
        for e in self.slice {
            e.foo(tmp);
        }
    }
}

Trying to compile it, the build fails with: LLVM ERROR: Not supported instr: <MCInst 271 <MCOperand Reg:42>>. Slight modifications of this example can halt the compiler in a (seemingly, I got bored after 15 minutes of waiting) infinite loop.

This is a minimized repro. What's especially interesting is that deleting the D variant causes the code to compile. However, on the original code, having the A-B-C variants still fails.

I am trying to build for an ESP32 using the 1.56.0.1 rustc version. Build fails both locally (version below) and on a freshly set-up ubuntu GHA runner.

❯ rustc +esp -vV
rustc 1.56.0-dev
binary: rustc
commit-hash: unknown
commit-date: unknown
host: x86_64-pc-windows-msvc
release: 1.56.0-dev
LLVM version: 12.0.1
bugadani commented 3 years ago

Repro project: https://github.com/bugadani/xtensa-rust-bug-repro

MabezDev commented 3 years ago

Thank you @bugadani! The repro project is very useful, hopefully we should be able to track this down relatively quickly.

MabezDev commented 3 years ago

Ouch, maybe not as simple as I thought. I can reproduce the error with your repo, however when I ask rustc to emit llvm ir it magically compiles...

Do you see the same behavior?

RUSTFLAGS='--emit=llvm-ir' cargo +esp-dev rustc --target=xtensa-esp32-espidf -Z build-std=std,panic_abort -Zbuild-std-features=panic_immediate_abort
bugadani commented 3 years ago

If this is similar enough:

❯ cargo +esp rustc --target=xtensa-esp32-espidf -Z build-std=std,panic_abort -Zbuild-std-features=panic_immediate_abort -- --emit=llvm-ir

Then no, I have this output, without any .ll files:

Compiling foolib v0.1.0 (C:\_OpenSource\idftest\foolib)
LLVM ERROR: Not supported instr: <MCInst 271 <MCOperand Reg:42>>
error: could not compile `foolib`
warning: build failed, waiting for other jobs to finish...
error: build failed
MabezDev commented 3 years ago

❯ cargo +esp rustc --target=xtensa-esp32-espidf -Z build-std=std,panic_abort -Zbuild-std-features=panic_immediate_abort -- --emit=llvm-ir

Did not emit .ll files for me either, but setting RUSTFLAGS did. Once it did, the LLVM error went away on my machine :thinking:.

bugadani commented 3 years ago

You are right, I see the same thing on my Windows machine. Interesting, I wouldn't have expected any difference between RUSTFLAGS and cli arguments.

MabezDev commented 2 years ago

Hi @bugadani, we've fixed this issue in LLVM and should be available in the next release! Will keep this open until its available publicly.

MabezDev commented 2 years ago

Thanks for your patience @bugadani, this should now be fixed in the 1.58 release of the fork (includes the fix inside our LLVM 13 release). Please let me know if you encounter any more issues :)

bugadani commented 2 years ago

Thanks! I'll return with some feedback as soon as the binaries are released. In the mean time, could you please link the commit that fixed the issue? For one, it should be a good reference here, and I'm also interested in what went wrong :) Thank you!

MabezDev commented 2 years ago

I'm glad you asked actually! I just realised the PR hasn't actually made it into the llvm13 release just yet. Will reopen this issue (sorry for the noise!).

As for what went wrong, the esp32 has hardware loop registers and in your test case it was possible to generate multiple loop entries which weren't being handled correctly in the backend.

bugadani commented 2 years ago

Yup, 1.58.0.0 loops without making progress. At least I cancelled the build after 38 minutes that usually takes like 5.

MabezDev commented 2 years ago

This is now fixed in the 1.59 release, the specif commit in LLVM is https://github.com/espressif/llvm-project/commit/d735e99d2b5d6befc9a22def69849991c52caf97. Thanks for your patience!