Skip to content
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

Tracking issue for booleans #1133

Closed
4 tasks
jyn514 opened this issue Oct 6, 2019 · 9 comments
Closed
4 tasks

Tracking issue for booleans #1133

jyn514 opened this issue Oct 6, 2019 · 9 comments
Labels
cranelift Issues related to the Cranelift code generator

Comments

@jyn514
Copy link
Contributor

jyn514 commented Oct 6, 2019

See vaguely #1110, #1045, and jyn514/saltwater#92. An easy way to do this might be to convert from bool -> int -> bool and back as suggested in https://github.com/CraneStation/cranelift/issues/922#issuecomment-523931097.

  • bnot
    Only occurs if you return the value (maybe would also happen for function calls?)
function u0:0() -> i32 system_v {
ebb0:
    v0 = iconst.i32 1
    v1 = icmp_imm ne v0, 0
    v2 = bnot v1
    v3 = bint.i32 v2
    return v3
}

codegen error: Compilation error: Verifier errors:
- inst2: v2 is a real GPR value defined by a ghost instruction
  • bnz
    Note: this only occurs when the values are i8, not for i32, and it occurs during codegen, not verification.
function u0:0() system_v {
ebb0:
    v0 = iconst.i8 0
    v1 = iconst.i8 0
    v2 = icmp ne v0, v1
    brz v2, ebb1
    return

ebb1:
    return
}

codegen error: Compilation error: Verifier errors:
- inst3: Branch must have an encoding
  • store
function u0:0() -> i32 system_v {
    ss0 = explicit_slot 1

ebb0:
    v0 = bconst.b1 true
    v1 = stack_addr.i64 ss0
    store v0, v1
    v2 = iconst.i32 0
    return v2
}

fatal: - inst2: has an invalid controlling type b1
  • bor
function u0:0() -> i32 system_v {
    ss0 = explicit_slot 1

ebb0:
    v0 = stack_addr.i64 ss0
    v1 = bconst.b1 true
    v2 = load.b1 v0
    v3 = bor v2, v1
    store v3, v0
    v4 = iconst.i32 0
    return v4
}

fatal: - inst2: has an invalid controlling type b1
- inst4: has an invalid controlling type b1
@bjorn3
Copy link
Contributor

bjorn3 commented Oct 6, 2019

iconst.b1 should be bconst.b1.

Edit: @jyn514 removed it from the list.

bjorn3 referenced this issue in bjorn3/cretonne Nov 23, 2019
abrown referenced this issue in bytecodealliance/cranelift Dec 4, 2019
* Legalize stack_{load,store}.i8, fixes #433
* Legalize select.i8, cc: #466
* Legalize brz.i8 and brnz.i8, cc: #1117
@EliSnow
Copy link

EliSnow commented Jan 3, 2020

bxor_not seems to also not work with a boolean value

@alexcrichton alexcrichton transferred this issue from bytecodealliance/cranelift Feb 28, 2020
@alexcrichton alexcrichton added the cranelift Issues related to the Cranelift code generator label Feb 28, 2020
@jviereck
Copy link

Looking for a good first bug to work on, do people thing this is a good one to get started hacking Cranelift? If so, could someone give me a pointer on how to start addressing this issue?

@bjorn3
Copy link
Contributor

bjorn3 commented Oct 24, 2020

We are currently in the process of switching to a new framework for backends. I am not quite sure what has already been implemented in the new backends and what hasn't.

@StackDoubleFlow
Copy link
Contributor

Is the process complete? bnot now panics on an unimplemented! during lowering.

@cfallin
Copy link
Member

cfallin commented Sep 30, 2021

Yes, we're currently using the new backend. It looks like the lowering for bnot for scalar boolean types exists for aarch64 but not for x86-64; a PR to add this on x86-64 would be a welcome contribution and a good first bug if anyone wants to try this!

(We still have some holes in supported-operand-types coverage for CLIF that is outside of what the Wasm frontend generates, and the Wasm frontend doesn't use Cranelift-level bools, so that's probably why this still isn't implemented. Sorry about that!)

@akirilov-arm
Copy link
Contributor

And looking at the other items in this issue - currently we do not allow loads and stores of scalar booleans; this has been discussed at length in #3205, together with other aspects of the boolean types.

@pnevyk
Copy link

pnevyk commented Dec 23, 2021

It seems that bnot no longer panics on x86_64 target since #3592 . I tried to allow x86_64 target in this runtest and it does not fail, while it fails before the PR.

Not sure if that was intentional given the ongoing (?) discussion on representation of b1.

@cfallin
Copy link
Member

cfallin commented May 4, 2022

Indeed (re: above), bools are fairly well supported now; I think we can probably close this tracking issue. The broader issue of what we want to do about bool representations in #3205 should continue regardless, as we still need to pin down some details of what we want at the CLIF-semantics level.

@cfallin cfallin closed this as completed May 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

No branches or pull requests

9 participants