Fix codegen issues for 32-bit targets #477
No reviewers
Labels
No Milestone
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: M-Labs/nac3#477
Loading…
Reference in New Issue
No description provided.
Delete Branch "fix/32bit-codegen-issues"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
--mx32
and--m64
flags to standalone for emulating 32-bit and enforcing 64-bitsize_t
types respectivelycheck_demo.sh
output_exception
(Emitted by clang warnings)cslice32
todemo.c
to better reflect actual data type oflist
andstr
size_t
usingTargetMachine
passed into LLVM (if not specified in command-line)size_t
on 64-bit targets.The primary cause of codegen errors is due to
nac3core
previously assuming that the bit width ofsize_t
types and the bit width of the native word of the target platform (which is the type ofsizeof
expressions) is the same. This is true withnac3standalone
(wheresize_t
is defaulted to the host environment, i.e. 64-bit) andnac3artiq
(wheresize_t
is defaulted to the cross-compilation target, i.e. 32-bit). However, with the introduction of 32-bitsize_t
tests on 64-bit platform, there are now mismatches between this. As such, casts were added to promotesize_t
to the target platform word size where needed.Fixes #469 - #476; Supersedes #468.
@ -62,16 +62,21 @@ void output_asciiart(int32_t x) {
}
}
struct cslice32 {
Uh, why?
I think this
struct cslice
is made to be identical to https://docs.rs/cslice/0.3.0/src/cslice/lib.rs.html#33-37:Extra info:
struct Exception
depends oncslice
like so:b0d2705c38/artiq/firmware/libeh/eh_artiq.rs (L1C1-L17C1)
Sure, but with this PR introduces two cslices, one with uint32 and one with usize. Looks messy if not incorrect.
@ -79,0 +84,4 @@
/// Generates code by setting `size_t` to 32 bits.
#[arg(long)]
mx32: bool,
Why the
x
?This has been removed, but for clarity, GCC has an
-mx32
flag for x86 targets which:Which better matches what we were doing back then, generating using 32-bit
size_t
but targeting 64-bit archs.Ack.
We don't have to (and should not) implement every GCC option. And compiling for 686 is a better match to what is happening when we target the (current) ARTIQ devices.
@ -277,0 +300,4 @@
(false, false) => None,
(false, true) => Some(32),
(true, false) => Some(64),
(true, true) => {
Quite messy. And also this isn't what
clang -m32
does - the latter selects 32-bit x86 ISA, so there is no need to name such flags in the same way. The previous CLI with-s
was better.Or just use the existing
--target
and build a complete 32-bit binary, this way we don't have non-standard edge cases with 32-bit size_t on 64-bit.fe43ec00fa
to7f07017fc6
@ -149,1 +149,4 @@
buildInputs = with pkgs; [
pkgsi686Linux.glibc
pkgsi686Linux.libgcc
pkgsi686Linux.binutils
Are all those three needed? My code was just an unrefined proof of concept.
Updated to remove these packages.
@ -26,0 +21,4 @@
./interpret_demo.py "$demo" > interpreted.log
echo "...... Trying NAC3's 32-bit code generator output"
./run_demo.sh -m32 --out run_32.log "${nac3args[@]}" "$demo"
Won't this break the automatic tests? They don't have the 686 packages unless the flake is updated.
nix build -L .#packages.x86_64-linux.nac3artiq
works with only(pkgs.wrapClangMulti llvmPackages_14.clang)
changed. I don't actually know why though.@ -65,3 +65,3 @@
struct cslice {
void *data;
usize len;
uint32_t len;
Is this correct?
Yes, both string slices and list slices use uint32_t as its length.
This contradicts https://docs.rs/cslice/latest/cslice/
This
cslice
struct is forlist
andstring
in NAC3 which are defined this way, not Rust'scslice
. Could you propose a more suitable name for this struct?Is there a reason not to follow the Rust cslice?
The firmware uses Rust cslice and this will become a problem when we start using it on 64-bit CPUs like Zynq MPSoC.
Because
list
andstr
are currently defined as such in the compiler. I guess the solution moving forward would be the change the definition oflist
andstr
in the compiler to have its size assize_t
instead?Edit: Or maybe not, seems like
list
already uses 32-bit ints and my assumption is incorrect.Yes, just like they are defined in any other serious compiler.
@ -278,0 +304,4 @@
.get_bit_width()
});
if !matches!(size_t, 32 | 64) {
This isn't only about size_t.
Sorry for not marking this PR as WIP. Please give me tomorrow to clean up this patchset before submitting it for review.
Fix codegen issues for 32-bit size_t on 64-bit targetsto WIP: Fix codegen issues for 32-bit size_t on 64-bit targets@ -278,0 +306,4 @@
if !matches!(size_t, 32 | 64) {
let mut cmd = CommandLineArgs::command();
cmd.error(ErrorKind::InvalidValue, "Only -m32 or -m64 is supported").exit();
I don't think we need this -m flag at all. It looks superfluous, complex, redundant, and error prone. Just use
-target
and then get the information such as the bitwidth of size_t by querying the LLVM target object.It's also not only about the bitwidth of size_t. The code suggests it is.
7f07017fc6
to3468f30985
WIP: Fix codegen issues for 32-bit size_t on 64-bit targetsto WIP: Fix codegen issues for 32-bit targetsOnly issue remaining is
mandelbrot.py
not working in 32-bit mode with-O0
.float32 vs. float64 precision?
I don't think we use
float32
anywhere in codegen. Might be worth investigating though.3468f30985
to0b28c19dd9
Looks like a rounding error to me at
if z_r*z_r + z_i*z_i > 4.0
.Under normal circumstances, that evaluates to
False
and will continue one iteration, but compiling withi386
target instead evaluates that toTrue
and terminates the loop early.Note that the x87 FPU internally has higher-precision registers with 80 bits. Might have to do with that perhaps. I think use of the higher precision can be disabled.
Works if I use
--mcpu=pentium4
to compile the module. Should I set that (or another CPU model supporting SSE) or use--target-features sse4.1
?The second option sounds more specific and therefore better.
0b28c19dd9
tof74e9de682
f74e9de682
tob655d2dd39
WIP: Fix codegen issues for 32-bit targetsto Fix codegen issues for 32-bit targetsb655d2dd39
to8c5ba37d09
@ -12,3 +12,3 @@
case "$1" in
--help)
echo "Usage: run_demo.sh [--help] [--out OUTFILE] [--lli] [--debug] -- [NAC3ARGS...]"
echo "Usage: run_demo.sh [--help] [--out OUTFILE] [--debug] [-m32] -- [NAC3ARGS...]"
call it
-i386
-m32
is generic, this one is not.This is generally an improvement over a pretty terrible situation, so merging now. The two issues still need fixing.