-
Notifications
You must be signed in to change notification settings - Fork 12.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
m68k-unknown-linux-gnu
: can't compile functions with certain return types
#89498
Comments
OK, I did some more testing and found that it crashes with the following return types:
Here's a minimal example: #![no_std]
#![feature(no_core, lang_items)]
#![no_core]
#[lang = "sized"]
pub trait Sized {}
fn llvm_test() -> (u64, bool) {
(24234324, false)
} |
m68k-unknown-linux-gnu
m68k-unknown-linux-gnu
: can't compile functions with certain return types
The m68k backend is in an early stage, so some issues are to be expected. If it's a LLVM issue, please file a bug in the LLVM upstream tracker against the M68k backend. |
I will check if the bug is present in upstream LLVM and file a bug if indicated, thanks! |
Those all look like cases where the ABI returns values indirectly (the usual thing on 32-bit architectures, where 64-bit values are returned in register pairs, but larger things indirectly), so the frontend should be adding an implicit sret argument to the function and representing it as returning void, with the backends always assuming that part of the ABI has been done correctly. What IR do you get for your example? |
This is the IR I get: *** IR Dump Before Module Verifier (verify) ***
; Function Attrs: nonlazybind uwtable
define { i64, i8 } @llvm_test() unnamed_addr #0 !dbg !6 {
start:
%0 = alloca { i64, i8 }, align 4
%1 = bitcast { i64, i8 }* %0 to i64*, !dbg !18
store i64 24234324, i64* %1, align 4, !dbg !18
%2 = getelementptr inbounds { i64, i8 }, { i64, i8 }* %0, i32 0, i32 1, !dbg !18
store i8 0, i8* %2, align 4, !dbg !18
%3 = getelementptr inbounds { i64, i8 }, { i64, i8 }* %0, i32 0, i32 0, !dbg !19
%4 = load i64, i64* %3, align 4, !dbg !19
%5 = getelementptr inbounds { i64, i8 }, { i64, i8 }* %0, i32 0, i32 1, !dbg !19
%6 = load i8, i8* %5, align 4, !dbg !19, !range !20
%7 = trunc i8 %6 to i1, !dbg !19
%8 = zext i1 %7 to i8, !dbg !19
%9 = insertvalue { i64, i8 } undef, i64 %4, 0, !dbg !19
%10 = insertvalue { i64, i8 } %9, i8 %8, 1, !dbg !19
ret { i64, i8 } %10, !dbg !19
} |
Yep, rustc frontend bug, should be: ; Function Attrs: nonlazybind uwtable
define void @llvm_test({ i64, i8 }* sret %result) unnamed_addr #0 !dbg !6 {
start:
%0 = alloca { i64, i8 }, align 4
%1 = bitcast { i64, i8 }* %0 to i64*, !dbg !18
store i64 24234324, i64* %1, align 4, !dbg !18
%2 = getelementptr inbounds { i64, i8 }, { i64, i8 }* %0, i32 0, i32 1, !dbg !18
store i8 0, i8* %2, align 4, !dbg !18
%3 = getelementptr inbounds { i64, i8 }, { i64, i8 }* %0, i32 0, i32 0, !dbg !19
%4 = load i64, i64* %3, align 4, !dbg !19
%5 = getelementptr inbounds { i64, i8 }, { i64, i8 }* %0, i32 0, i32 1, !dbg !19
%6 = load i8, i8* %5, align 4, !dbg !19, !range !20
%7 = trunc i8 %6 to i1, !dbg !19
%8 = zext i1 %7 to i8, !dbg !19
%9 = insertvalue { i64, i8 } undef, i64 %4, 0, !dbg !19
%10 = insertvalue { i64, i8 } %9, i8 %8, 1, !dbg !19
store { i64, i8 } %10, { i64, i8 }* %result
ret void
} |
Alright, thanks for the example of correct IR; I'll see if I can figure out how to fix this in rustc. |
OK, I've been working on fixes for this, but I haven't figured out a way to test anything since rustc won't build because of another LLVM error, which seems to be related to the left shift operator when used on 64bit integers:
This is happening in the |
@AnnikaCodes Thanks a lot for investigating this and thanks a lot to @jrtc27 for figuring out so quickly what the problem is! |
The left-shift on 64-bit ints crash is an LLVM bug (it can be reproduced in C); I've reported it to the LLVM project. |
Bug has been fixed in LLVM llvm/llvm-project#51461. |
@AnnikaCodes If you use LLVM from git and libc with the patches from here rust-lang/libc#2680 and here rust-lang/libc@7a027d3, you should be able to work on this issue. If I understood correctly, you already have a potential fix for this issue? |
Isn't that what |
@glaubitz I believe the problem here is that rust only uses target-specific ABI adjustment when using C (or other foreign) ABI, but not for Rust ABI. I assume that for other targets, LLVM will handle this gracefully even if it does not match the platform C ABI. |
Thanks for the explanation. I have to admit that I currently have no idea how to fix this issue. I tried playing with variations in the ABI code for m68k in Rust a bit to modify the function return behavior but that didn't make any difference. |
I need to set up my Rust development environment again; I think I know how to fix this based on jrtc's IR, but I was never able to test it so no promises there :) |
Can you give me a hint where the Rust code is located that generates the return code? |
@glaubitz See rust/compiler/rustc_middle/src/ty/layout.rs Line 3177 in f90b06d
Possibly a target option could be added to always use the C ABI for a target, if LLVM doesn't support non-C ABI for it. |
Ah, that somewhat rings a bell. I think I've heard that before that one had to enforce the C-ABI for a target. Now I just need to remember where ;). |
I have tried this hack now and it seems to fix the issue for me: diff --git a/compiler/rustc_middle/src/ty/layout.rs b/compiler/rustc_middle/src/ty/layout.rs
index 6d4178c3e75..1fd875c1b7a 100644
--- a/compiler/rustc_middle/src/ty/layout.rs
+++ b/compiler/rustc_middle/src/ty/layout.rs
@@ -3223,6 +3223,15 @@ fn fn_abi_adjust_for_abi(
return;
}
+
+ Abi::ScalarPair { .. }
+ if self.tcx.sess.target.arch == "m68k" =>
+ {
+ arg.make_indirect();
+ return;
+ }
+
+
_ => return,
}
It just fails with a different error now which could be due to the lacks of support for atomic operations in the M68k LLVM backend:
See this related LLVM bug: llvm/llvm-project#48236. |
I have been looking at the corresponding Rust code in if abi == SpecAbi::Rust
|| abi == SpecAbi::RustCall
|| abi == SpecAbi::RustIntrinsic
|| abi == SpecAbi::PlatformIntrinsic
{
let fixup = |arg: &mut ArgAbi<'tcx, Ty<'tcx>>| {
if arg.is_ignore() {
return;
}
match arg.layout.abi {
Abi::Aggregate { .. } => {}
// This is a fun case! The gist of what this is doing is
// that we want callers and callees to always agree on the
// ABI of how they pass SIMD arguments. If we were to *not*
// make these arguments indirect then they'd be immediates
// in LLVM, which means that they'd used whatever the
// appropriate ABI is for the callee and the caller. That
// means, for example, if the caller doesn't have AVX
// enabled but the callee does, then passing an AVX argument
// across this boundary would cause corrupt data to show up.
//
// This problem is fixed by unconditionally passing SIMD
// arguments through memory between callers and callees
// which should get them all to agree on ABI regardless of
// target feature sets. Some more information about this
// issue can be found in #44367.
//
// Note that the platform intrinsic ABI is exempt here as
// that's how we connect up to LLVM and it's unstable
// anyway, we control all calls to it in libstd.
Abi::Vector { .. }
if abi != SpecAbi::PlatformIntrinsic
&& self.tcx.sess.target.simd_types_indirect =>
{
arg.make_indirect();
return;
}
_ => return,
}
// Pass and return structures up to 2 pointers in size by value, matching `ScalarPair`.
// LLVM will usually pass these in 2 registers, which is more efficient than by-ref.
let max_by_val_size = Pointer.size(self) * 2;
let size = arg.layout.size;
if arg.layout.is_unsized() || size > max_by_val_size {
arg.make_indirect();
} else {
// We want to pass small aggregates as immediates, but using
// a LLVM aggregate type for this leads to bad optimizations,
// so we pick an appropriately sized integer type instead.
arg.cast_to(Reg { kind: RegKind::Integer, size });
}
};
fixup(&mut fn_abi.ret);
for arg in &mut fn_abi.args {
fixup(arg);
}
} else {
fn_abi.adjust_for_foreign_abi(self, abi)?;
} If I read the code correctly, the code will never get beyond the Can someone with more Rust knowledge explain the code? |
The code applies to aggregates and sometimes vectors. Scalars and pairs don't (currently) need any fixing up. I imagine the m68k backend needs to be altered to permit pairs as arguments and return values, but I could see an argument to be made that, if the ABI doesn't let you pass pairs in registers, that should be the frontend's responsibility to lower. |
Ah, that actually makes sense and that explains why my hack above fixes the issue. |
Do you still want me to work on this? |
I'm not able to compile the following with your diff: pub struct Struct {
a: i64,
b: i32,
}
fn t() -> Struct {
Struct { a: 1000, b: 3000 }
} The following error is produced:
I'm not sure why this is; here is the produced LLVM IR with your diff: ; simple::t
; Function Attrs: nonlazybind uwtable
define internal void @_ZN6simple1t17h9b9464898cd4c360E({ i64, i32 }* noalias nocapture noundef sret({ i64, i32 }) dereferenceable(12) %0) unnamed_addr #0 {
start:
%1 = bitcast { i64, i32 }* %0 to i64*
store i64 1000, i64* %1, align 4
%2 = getelementptr inbounds { i64, i32 }, { i64, i32 }* %0, i32 0, i32 1
store i32 3000, i32* %2, align 4
ret void
} and the IR from compiling a similar function in C with %struct.Struct = type { i64, i32, [4 x i8] }
; Function Attrs: noinline nounwind optnone
define dso_local void @t(%struct.Struct* noalias sret(%struct.Struct) align 8 %0) #0 {
%2 = getelementptr inbounds %struct.Struct, %struct.Struct* %0, i32 0, i32 0
store i64 1000, i64* %2, align 8
%3 = getelementptr inbounds %struct.Struct, %struct.Struct* %0, i32 0, i32 1
store i32 3000, i32* %3, align 8
ret void
} |
The IR you pasted is for a totally different function |
@dawn-minion The M68k backend in LLVM is still considered experimental and there are still a number of bugs. I recommend that you always try to build the Rust compiler with LLVM from git since there are regularly coming new improvements in for the M68k backend. FWIW, we have both a Patreon and an OpenCollective campaign to support the developers of the M68k LLVM backend, so if you're really interested in getting the backend working, please consider supporting either of the campaigns. |
Applying the
Trying with
|
I think the issue here is simply that Then I seem to get the error with Is that one supposed to have been fixed now that llvm/llvm-project#48236 is closed? |
Sounds good. Thanks for catching this. Looking forward for your patch!
Yes, support for atomic operations were recently implemented and there are also already patches for TLS support. |
I have used the following change now and it works for me: diff --git a/llvm/lib/Target/M68k/M68kISelLowering.cpp b/llvm/lib/Target/M68k/M68kISelLowering.cpp
index 0ee3a07e15f4..7e6cb46406e8 100644
--- a/llvm/lib/Target/M68k/M68kISelLowering.cpp
+++ b/llvm/lib/Target/M68k/M68kISelLowering.cpp
@@ -1058,6 +1058,15 @@ SDValue M68kTargetLowering::LowerFormalArguments(
// Return Value Calling Convention Implementation
//===----------------------------------------------------------------------===//
+
+bool M68kTargetLowering::CanLowerReturn(
+ CallingConv::ID CallConv, MachineFunction &MF, bool isVarArg,
+ const SmallVectorImpl<ISD::OutputArg> &Outs, LLVMContext &Context) const {
+ SmallVector<CCValAssign, 16> RVLocs;
+ CCState CCInfo(CallConv, isVarArg, MF, RVLocs, Context);
+ return CCInfo.CheckReturn(Outs, RetCC_M68k);
+}
+
SDValue
M68kTargetLowering::LowerReturn(SDValue Chain, CallingConv::ID CCID,
bool IsVarArg,
diff --git a/llvm/lib/Target/M68k/M68kISelLowering.h b/llvm/lib/Target/M68k/M68kISelLowering.h
index f9037e7ff497..d43160fe48d2 100644
--- a/llvm/lib/Target/M68k/M68kISelLowering.h
+++ b/llvm/lib/Target/M68k/M68kISelLowering.h
@@ -257,6 +257,11 @@ private:
SDValue LowerCall(CallLoweringInfo &CLI,
SmallVectorImpl<SDValue> &InVals) const override;
+ bool CanLowerReturn(CallingConv::ID CallConv, MachineFunction &MF,
+ bool isVarArg,
+ const SmallVectorImpl<ISD::OutputArg> &Outs,
+ LLVMContext &Context) const override;
+
/// Lower the result values of a call into the
/// appropriate copies out of appropriate physical registers.
SDValue LowerReturn(SDValue Chain, CallingConv::ID CCID, bool IsVarArg, @ids1024 Do you want to post the patch yourself or shall I? Furthermore, in order to use atomics, we will have to raise the baseline to M68020: diff --git a/compiler/rustc_target/src/spec/m68k_unknown_linux_gnu.rs b/compiler/rustc_target/src/spec/m68k_unknown_linux_gnu.rs
index ebd74012dcd..9bcd56bed00 100644
--- a/compiler/rustc_target/src/spec/m68k_unknown_linux_gnu.rs
+++ b/compiler/rustc_target/src/spec/m68k_unknown_linux_gnu.rs
@@ -3,6 +3,7 @@
pub fn target() -> Target {
let mut base = super::linux_gnu_base::opts();
+ base.cpu = "M68020".into();
base.max_atomic_width = Some(32);
Target { I will open a PR for that. |
I submitted a patch once I figured out how to add a test for it in LLVM IR: https://reviews.llvm.org/D148856 Ah, so it needs to target the M68020 for atomics? https://www.debian.org/ports/m68k says it requires a 68020, so this probably isn't an issue for the Linux target. But presumably it should be possible to compile libcore for a base 68000. I guess if it doesn't have atomic operations that would use With the change to
Edit: llvm/llvm-project#60554 |
Yes, this is expected because diff --git a/llvm/lib/Target/M68k/M68kExpandPseudo.cpp b/llvm/lib/Target/M68k/M68kExpandPseudo.cpp
index 5383d3107955..8ecd5724354d 100644
--- a/llvm/lib/Target/M68k/M68kExpandPseudo.cpp
+++ b/llvm/lib/Target/M68k/M68kExpandPseudo.cpp
@@ -251,9 +251,9 @@ bool M68kExpandPseudo::ExpandMI(MachineBasicBlock &MBB,
MIB = BuildMI(MBB, MBBI, DL, TII->get(M68k::RTS));
} else if (isUInt<16>(StackAdj)) {
- if (STI->atLeastM68020()) {
- llvm_unreachable("RTD is not implemented");
- } else {
+ // if (STI->atLeastM68020()) {
+ // llvm_unreachable("RTD is not implemented");
+ // } else {
// Copy PC from stack to a free address(A0 or A1) register
// TODO check if pseudo expand uses free address register
BuildMI(MBB, MBBI, DL, TII->get(M68k::MOV32aj), M68k::A1)
@@ -269,7 +269,7 @@ bool M68kExpandPseudo::ExpandMI(MachineBasicBlock &MBB,
// RTS
BuildMI(MBB, MBBI, DL, TII->get(M68k::RTS));
- }
+ // }
} else {
// TODO: RTD can only handle immediates as big as 2**16-1.
// If we need to pop off bytes before the return address, we We can either implement |
FWIW, we can introduce a new target for this case similar to the various |
If it couldn't fit the return value in two registers, this caused an error during codegen. It seems this method is implemented in other backends but not here, and allows it to pass return values in memory when it isn't able to do so in registers. Seems to fix compilation of Rust code with certain return types: rust-lang/rust#89498 Differential Revision: https://reviews.llvm.org/D148856
Since this fix has now landed in LLVM, I guess we can close this issue. @AnnikaCodes Could you close this issue? |
With https://reviews.llvm.org/D149034 and previous patches, using /lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp
index 78ac0c27d720..f4e0b10d3349 100644
--- a/llvm/lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp
@@ -508,7 +508,7 @@ `(SDNode *N, unsigned &NestLevel, unsigned &MaxNest,
BestMaxNest = MyMaxNest;
}
}
- assert(Best);
+ //assert(Best);
MaxNest = BestMaxNest;
return Best;
}
diff --git a/llvm/lib/Target/M68k/M68kISelLowering.cpp b/llvm/lib/Target/M68k/M68kISelLowering.cpp
index c37ec304bf19..465c39d9cf13 100644
--- a/llvm/lib/Target/M68k/M68kISelLowering.cpp
+++ b/llvm/lib/Target/M68k/M68kISelLowering.cpp
@@ -75,9 +75,9 @@ M68kTargetLowering::M68kTargetLowering(const M68kTargetMachine &TM,
setOperationAction(ISD::MUL, MVT::i8, Promote);
setOperationAction(ISD::MUL, MVT::i16, Legal);
- if (Subtarget.atLeastM68020())
- setOperationAction(ISD::MUL, MVT::i32, Legal);
- else
+ //if (Subtarget.atLeastM68020())
+ // setOperationAction(ISD::MUL, MVT::i32, Legal);
+ //else
setOperationAction(ISD::MUL, MVT::i32, LibCall);
setOperationAction(ISD::MUL, MVT::i64, LibCall);
@@ -183,6 +183,12 @@ M68kTargetLowering::M68kTargetLowering(const M68kTargetMachine &TM,
ISD::ATOMIC_SWAP,
},
{MVT::i8, MVT::i16, MVT::i32}, LibCall);
+ setOperationAction(
+ {
+ ISD::ATOMIC_LOAD,
+ ISD::ATOMIC_STORE,
+ },
+ {MVT::i8, MVT::i16, MVT::i32}, Expand);
setMinFunctionAlignment(Align(2));
}
diff --git a/llvm/lib/Target/M68k/M68kInstrInfo.cpp b/llvm/lib/Target/M68k/M68kInstrInfo.cpp
index 15b97ba25af4..fe597389dc7a 100644
--- a/llvm/lib/Target/M68k/M68kInstrInfo.cpp
+++ b/llvm/lib/Target/M68k/M68kInstrInfo.cpp
@@ -672,12 +672,12 @@ void M68kInstrInfo::copyPhysReg(MachineBasicBlock &MBB,
bool ToSR = DstReg == M68k::SR;
if (FromCCR) {
- assert(M68k::DR8RegClass.contains(DstReg) &&
- "Need DR8 register to copy CCR");
+ //assert(M68k::DR8RegClass.contains(DstReg) &&
+ // "Need DR8 register to copy CCR");
Opc = M68k::MOV8dc;
} else if (ToCCR) {
- assert(M68k::DR8RegClass.contains(SrcReg) &&
- "Need DR8 register to copy CCR");
+ //assert(M68k::DR8RegClass.contains(SrcReg) &&
+ // "Need DR8 register to copy CCR");
Opc = M68k::MOV8cd;
} else if (FromSR || ToSR)
llvm_unreachable("Cannot emit SR copy instruction"); The |
Or well, the |
I think this is likely caused by this issue: https://discourse.llvm.org/t/finding-the-start-sdnode-of-a-call-sequence/62806 I have a few ideas on how to fix it and will probably start to work on this issue recently. |
We're at the point now, any idea how to move forward? |
https://reviews.llvm.org/D152120 addressed the multiply issue, so that's another of the workarounds from this diff rendered unnecessary. I don't really understand the SelectionDAG issue. Not sure if @mshockwave has made any progress solving that one? That seems to be the most complicated thing here. The I've tried to investigate the After that, libcore should compile. And I think there were one or two other small issue with libstd. Then it would be possible to test actual applications like ripgrep. |
The |
Sadly it's not just the asserts, at least for the M68000. I'm trying to build libcore for the M68000 with the patches outlined in this thread and if I remove the asserts from Edit: llvm/llvm-project#85686 |
If it couldn't fit the return value in two registers, this caused an error during codegen. It seems this method is implemented in other backends but not here, and allows it to pass return values in memory when it isn't able to do so in registers. Seems to fix compilation of Rust code with certain return types: rust-lang/rust#89498 Differential Revision: https://reviews.llvm.org/D148856
rumor has it that @knickish has managed to build libcore with the upstream LLVM. |
Oh, I think only after patching: llvm/llvm-project#114714 |
Core should build with current upstream main, but asserts have to be off. Oncelock has a method that hits that assert, but the actual assembly should be ok for it regardless as the selected instruction hasn't changed even with the patch you mentioned |
That would be a major step forward. Congratulations @knickish! Achieving this milestone has been my goal for years. Let's get your LLVM fixes reviewed and merged, @mshockwave is our primary maintainer for the M68k backend. |
I think the m68k support for Rust is really interesting, and I've been trying to make a project with it. However, I'm unable to build the core library, which makes it really hard to actually use Rust.
You can replicate this by running
cargo build -Zbuild-std=core --target m68k-unknown-linux-gnu
.I expected it to build successfully.
Instead, I get an LLVM error:
I think this has something to do with functions that return
u64
andu128
?Meta
rustc --version --verbose
:Backtrace
The text was updated successfully, but these errors were encountered: