Skip to content
This repository was archived by the owner on Jun 26, 2020. It is now read-only.

[codegen] reintroduce support for carry and borrow instructions in RI…#1005

Merged
bnjbvr merged 5 commits intobytecodealliance:masterfrom
ryzokuken:fix/riscv/flags
Sep 13, 2019
Merged

[codegen] reintroduce support for carry and borrow instructions in RI…#1005
bnjbvr merged 5 commits intobytecodealliance:masterfrom
ryzokuken:fix/riscv/flags

Conversation

@ryzokuken
Copy link
Contributor

…SC ISAs

Reintroduce support for iadd carry variants and isub borrow variants for
RISC ISAs which had been removed in
#961 and
#962 because of the lack
of a proper flags register in RISC architectures.

/cc @wingo @caitp

@sunfishcode @bnjbvr there's a slight problem. x86 and RISC seem to be reusing both narrow and expand so I cannot choose between the two different versions of the instructions (eg: since both x86 and RISC use narrow to expand iadd.i64, one would need to choose between iadd_c* and iadd_ifc*). I believe creating a specialized legalization file for riscv in meta and adding the legalization specifically should help.

@bnjbvr
Copy link
Member

bnjbvr commented Sep 9, 2019

Thanks for looking into this! Our legalization groups have historically grown in a very ad-hoc way, and this is proof they don't quite make sense. We could split them into smaller ones, and chain them together to keep the things working almost the same way. It would be interesting to make a legalization group that's not specific to RISC but to platforms which don't have flags, for one thing.

…SC ISAs

Reintroduce support for iadd carry variants and isub borrow variants for
RISC ISAs which had been removed in
bytecodealliance#961 and
bytecodealliance#962 because of the lack
of a proper flags register in RISC architectures.
@ryzokuken
Copy link
Contributor Author

Added a new legalization group and rebased. IIUC, this should've passed, but this clearly doesn't: it throws

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: "line 4: fail ti on typeof_c <: own_5: Error: empty type created when unifying own_5 and iflags"', src/libcore/result.rs:999:5

@bnjbvr @sunfishcode what might be wrong?

@ryzokuken
Copy link
Contributor Author

@bjorn3 sorry, the new recipes were added rather hastily, and I didn't put a lot of thought into them. The updated version should look much more accurate. That said, I'm still getting thread 'main' panicked at 'assertion failed: !apply_target.inst().operands_in[i].is_value()', cranelift-codegen/meta/src/cdsl/xform.rs:220:17, investigating that.

vec![
def!((a, c1) = iadd_cout(x, y)),
def!(c_int = bint(c1)),
def!(c = ifcmp(uimm32_0, c_int)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would turn overflow into the ne flag, instead of the carry flag.

Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great, thanks! Very small comments, I'm happy to address them for you in a subsequent small PR, if you don't want to deal with those now.

@bnjbvr
Copy link
Member

bnjbvr commented Sep 13, 2019

(Actually added those for you! If the build passes, I'll squash everything.)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants