M-Labs/nac3
M-Labs/nac3
8
1
Fork 13
Code Issues 95 Pull Requests 4 Releases Wiki Activity

Fix several missing i1/i8 casts #525

Merged
sb10q merged 2 commits from fix/misc-fix into master 2024-08-29 16:36:32 +08:00
Conversation 2 Commits 2 Files Changed 2 +24 -20
2 changed files with 24 additions and 20 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
nac3core/src/codegen/expr.rs
View File

@ -2385,7 +2385,10 @@ pub fn gen_cmpop_expr_with_values<'ctx, G: CodeGenerator>(
})
.map(BasicValueEnum::into_int_value)?;
Ok(ctx.builder.build_not(cmp, "").unwrap())
Ok(ctx.builder.build_not(
generator.bool_to_i1(ctx, cmp),
"",
).unwrap())
},
|_, ctx| {
let bb = ctx.builder.get_insert_block().unwrap();

39
nac3core/src/codegen/stmt.rs
View File

@ -174,6 +174,14 @@ pub fn gen_assign<'ctx, G: CodeGenerator>(
}
}
let val = value.to_basic_value_enum(ctx, generator, target.custom.unwrap())?;
// Perform i1 <-> i8 conversion as needed
sb10q commented 2024-08-28 22:00:21 +08:00
Outdated
Review
Copy Link

Do we need i1 at all?
IIRC it just creates problems and does not solve any. Another LLVM design flaw.

Do we need i1 at all? IIRC it just creates problems and does not solve any. Another LLVM design flaw.
derppening commented 2024-08-28 22:07:04 +08:00
Outdated
Review
Copy Link

All boolean instructions (e.g. icmp) return i1, and LLVM's ABI uses i1 for boolean parameters in functions. I would prefer to follow these conventions as we need these casts either way.

All boolean instructions (e.g. `icmp`) return `i1`, and LLVM's ABI uses `i1` for boolean parameters in functions. I would prefer to follow these conventions as we need these casts either way.
sb10q commented 2024-08-29 06:39:12 +08:00
Outdated
Review
Copy Link

Hmm OK, but then I don't remember why we can't just use i1 for the Python bool type. Then we would not need any casts?

Hmm OK, but then I don't remember why we can't just use i1 for the Python bool type. Then we would not need any casts?
lyken commented 2024-08-29 09:40:58 +08:00
Outdated
Review
Copy Link

On i1 vs i8 for bools, is this the relevant commit? 31dcd2dde9

core: Use i8 for boolean variable allocation

In LLVM, i1 represents a 1-byte integer with a single valid bit; The
rest of the 7 upper bits are undefined. This causes problems when
using these variables in memory operations (e.g. memcpy/memmove as
needed by List slicing and assignment).

We fix this by treating all local boolean variables as i8 so that they
are well-defined for memory operations. Function ABIs will continue to
use i1, as memory operations cannot be directly performed on function
arguments or return types, instead they are always converted back into
local boolean variables (which are i8s anyways).

Fixes #315.

On i1 vs i8 for bools, is this the relevant commit? https://git.m-labs.hk/M-Labs/nac3/commit/31dcd2dde9ac088adfadec4b8ddc844276306c55 > core: Use i8 for boolean variable allocation > > In LLVM, i1 represents a 1-byte integer with a single valid bit; The > rest of the 7 upper bits are undefined. This causes problems when > using these variables in memory operations (e.g. memcpy/memmove as > needed by List slicing and assignment). > > We fix this by treating all local boolean variables as i8 so that they > are well-defined for memory operations. Function ABIs will continue to > use i1, as memory operations cannot be directly performed on function > arguments or return types, instead they are always converted back into > local boolean variables (which are i8s anyways). > > Fixes #315.
sb10q commented 2024-08-29 10:46:11 +08:00
Outdated
Review
Copy Link

Right, that's the one, and the reason to complain about LLVM.

Right, that's the one, and the reason to complain about LLVM.
let val = if ctx.unifier.unioned(target.custom.unwrap(), ctx.primitives.bool) {
generator.bool_to_i8(ctx, val.into_int_value()).into()
} else {
val
};
ctx.builder.build_store(ptr, val).unwrap();
}
};
@ -1665,6 +1673,18 @@ pub fn gen_return<G: CodeGenerator>(
} else {
None
};
// Remap boolean return type into i1
let value = value.map(|ret_val| {
let expected_ty = func.get_type().get_return_type().unwrap();
if matches!(expected_ty, BasicTypeEnum::IntType(ty) if ty.get_bit_width() == 1) {
generator.bool_to_i1(ctx, ret_val.into_int_value()).into()
} else {
ret_val
}
});
if let Some(return_target) = ctx.return_target {
if let Some(value) = value {
ctx.builder.build_store(ctx.return_buffer.unwrap(), value).unwrap();
@ -1675,25 +1695,6 @@ pub fn gen_return<G: CodeGenerator>(
ctx.builder.build_store(ctx.return_buffer.unwrap(), value.unwrap()).unwrap();
ctx.builder.build_return(None).unwrap();
} else {
// Remap boolean return type into i1
let value = value.map(|v| {
let expected_ty = func.get_type().get_return_type().unwrap();
let ret_val = v.as_basic_value_enum();
if expected_ty.is_int_type() && ret_val.is_int_value() {
let ret_type = expected_ty.into_int_type();
let ret_val = ret_val.into_int_value();
if ret_type.get_bit_width() == 1 && ret_val.get_type().get_bit_width() != 1 {
generator.bool_to_i1(ctx, ret_val)
} else {
ret_val
}
.into()
} else {
ret_val
}
});
let value = value.as_ref().map(|v| v as &dyn BasicValue);
ctx.builder.build_return(value).unwrap();
}