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
derppening commented 2024-08-28 15:38:50 +08:00
Collaborator
Copy Link
No description provided.
derppening requested review from sb10q 2024-08-28 15:38:53 +08:00
sb10q reviewed 2024-08-28 22:00:21 +08:00
nac3core/src/codegen/stmt.rs Outdated
@ -175,2 +175,4 @@
}
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
Owner
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
Author
Collaborator
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
Owner
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
Collaborator
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
Owner
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.
sb10q reviewed 2024-08-29 10:47:42 +08:00
nac3core/src/codegen/stmt.rs Outdated
@ -177,0 +187,4 @@
let llvm_ptr_elem_ty = llvm_ptr_elem_ty.into_int_type().get_bit_width();
match (llvm_val_ty, llvm_ptr_elem_ty) {
(v_bits, elem_bits) if v_bits == elem_bits && matches!(v_bits, 1 | 8) => val,
sb10q commented 2024-08-29 10:47:42 +08:00
Owner
Copy Link

This is a bit of a shotgun solution. Can't we be more subtle and just have the casts explicitly where they are needed?

This is a bit of a shotgun solution. Can't we be more subtle and just have the casts explicitly where they are needed?
sb10q commented 2024-08-29 10:50:47 +08:00
Owner
Copy Link

How about using i1 for bools instead, and casting them when calling foreign functions?

Then information about what is a bool stays in the LLVM IR.

How about using i1 for bools instead, and casting them when calling foreign functions? Then information about what is a bool stays in the LLVM IR.
derppening force-pushed fix/misc-fix from a2922b24e0 to 7e273d93b0 2024-08-29 16:19:15 +08:00 Compare
derppening commented 2024-08-29 16:20:06 +08:00
Author
Collaborator
Copy Link

Removed a lot of explicit checks as they are already handled in bool_to_i1 and bool_to_i8.

Removed a lot of explicit checks as they are already handled in `bool_to_i1` and `bool_to_i8`.
derppening force-pushed fix/misc-fix from 7e273d93b0 to 51264996b0 2024-08-29 16:26:30 +08:00 Compare
sb10q merged commit 8c540d1033 into master 2024-08-29 16:36:32 +08:00
sb10q deleted branch fix/misc-fix 2024-08-29 16:36:33 +08:00
Sign in to join this conversation.
Reviewers
No reviewers
sb10q
Labels
Clear labels   
future-extension

   
high-priority

   
low-priority

   
UROP0622
No Label
future-extension
high-priority
low-priority
UROP0622
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: M-Labs/nac3#525
No description provided.
Delete Branch "fix/misc-fix"

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?