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

[codegen] add encodings for iadd carry variants#961

Merged
bnjbvr merged 9 commits intobytecodealliance:masterfrom
ryzokuken:feature/86-32/iadd
Sep 5, 2019
Merged

[codegen] add encodings for iadd carry variants#961
bnjbvr merged 9 commits intobytecodealliance:masterfrom
ryzokuken:feature/86-32/iadd

Conversation

@ryzokuken
Copy link
Contributor

@ryzokuken ryzokuken commented Aug 31, 2019

Add encodings for iadd carry variants (iadd_cout, iadd_cin, iadd_carry)
for x86_32, enabling the legalization for iadd.i64 to work.

Bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1576675
Bug: https://github.com/CraneStation/cranelift/issues/765

/cc @bnjbvr @wingo @caitp

@ryzokuken
[codegen] add encodings for iadd carry variants
a315cb3 
Add encodings for iadd carry variants (iadd_cout, iadd_cin, iadd_carry)
for x86_32, enabling the legalization for iadd.i64 to work.

Bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1576675
Bug: https://github.com/CraneStation/cranelift/issues/765
@ryzokuken ryzokuken mentioned this pull request Aug 31, 2019
Merged
bjorn3
bjorn3 reviewed Sep 1, 2019
View reviewed changes
@bjorn3
Copy link
Contributor

bjorn3 commented Sep 1, 2019

Could you please add tests?

@ryzokuken
Copy link
Contributor Author

ryzokuken commented Sep 1, 2019

@bjorn3 I'm running tests locally, but I'm afraid I'll need #957 to merge into master before adding tests in this since I'd need iconst.i64 for that.

@ryzokuken
Copy link
Contributor Author

ryzokuken commented Sep 1, 2019

There is an issue though:

function u0:1(i32 [0], i32 [4]) -> i32 [%rax], i32 [%rdx] system_v {
    ss0 = incoming_arg 4, offset 0
    ss1 = incoming_arg 4, offset 4

                                ebb0(v4: i32 [ss0], v5: i32 [ss1]):
[-,-]                               v1 = iconcat v4, v5
[-,-]                               v3 = iadd_imm v1, 42
                                    v8, v9 = isplit v3
[Op1ret#c3]                         return v8, v9
;~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
; error: inst2: v8 is a ghost value used by a real [Op1ret#c3] instruction

}

; 1 verifier error detected (see above). Compilation aborted.

@bjorn3
Copy link
Contributor

bjorn3 commented Sep 1, 2019

I'll need #957 to merge into master before adding tests in this since I'd need iconst.i64 for that.

You could take 64bit as function arguments. You could also explicitly test iadd_c*.i32 instead of iadd.i64.

bnjbvr
bnjbvr suggested changes Sep 2, 2019
View reviewed changes
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.

You're going into the right direction! Since iadd_cin/cout have never been properly encoded, I think the right fix here is actually to change the instruction's format itself.

@bnjbvr
Copy link
Member

bnjbvr commented Sep 2, 2019

There is an issue though:

Yeah, that's simple preopt trying to optimize iadd(iconst, iconst) into iadd_imm(iconst). We should disable this when the result type is wider than the native pointer type, and we should maybe split the expand legalization group into a group of default legalizations (iadd_imm -> iconst + iadd), and things that are really expansions.

In the meanwhile, @bjorn3 's suggestion of using parameters sounds great for testing.

@ryzokuken
[codegen] remove support for iadd carry variants on riscv
38c612d 
Previously, the carry variants of iadd (iadd_cin, iadd_cout and
iadd_carry) were being legalized for isa/riscv since RISC architectures
lack a flags register.

This forced us to return and accept booleans for these operations, which
proved to be problematic and inconvenient, especially for x86.

This commit removes support for said statements and all dependent
statements for isa/riscv so that we can work on a better legalization
strategy in the future.
@ryzokuken
[codegen] change operand type from bool to iflag for iadd carry variants
4d9f584 
The type of the carry operands for the carry variants of the iadd
instruction (iadd_cin, iadd_cout, iadd_carry) was bool for compatibility
reasons for isa/riscv. Since support for these instructions on RISC
architectures has been temporarily suspended, we can safely change the
type to iflags.
@ryzokuken
fixup! [codegen] add encodings for iadd carry variants
b4241ed
@ryzokuken
fixup! [codegen] add encodings for iadd carry variants
f593884
bjorn3
bjorn3 reviewed Sep 4, 2019
View reviewed changes
bjorn3
bjorn3 reviewed Sep 4, 2019
View reviewed changes
bjorn3
bjorn3 reviewed Sep 4, 2019
View reviewed changes
@bjorn3
Copy link
Contributor

bjorn3 commented Sep 4, 2019

@ryzokuken I think you forgot to stage a file in the last commit. The operand type for iadd carry variants hasn't been changed.

@ryzokuken
Copy link
Contributor Author

ryzokuken commented Sep 4, 2019

@bjorn3 the last commit is a fixup? I changed the type in the original commit (4d9f584)

@bjorn3
Copy link
Contributor

bjorn3 commented Sep 4, 2019

Didn't see that commit :)

@bnjbvr bnjbvr self-requested a review September 5, 2019 09:35
bnjbvr
bnjbvr approved these changes Sep 5, 2019
View reviewed changes
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.

Looks great to me, thanks for adding tests!

@bnjbvr bnjbvr merged commit 3e71ee0 into bytecodealliance:master Sep 5, 2019
@bnjbvr
Copy link
Member

bnjbvr commented Sep 5, 2019

(I squashed all the commits together because Github doesn't do autosquash on merges; next time feel free to autosquash locally before pushing!)

@ryzokuken ryzokuken deleted the feature/86-32/iadd branch September 5, 2019 15:48
ryzokuken added a commit to ryzokuken/cranelift that referenced this pull request Sep 9, 2019
@ryzokuken
[codegen] reintroduce support for carry and borrow instructions in RI…
c962735 
…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 ryzokuken mentioned this pull request Sep 9, 2019
Merged
ryzokuken added a commit to ryzokuken/cranelift that referenced this pull request Sep 12, 2019
@ryzokuken
[codegen] reintroduce support for carry and borrow instructions in RI…
0ff4c0e 
…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.
bnjbvr pushed a commit that referenced this pull request Sep 13, 2019
@ryzokuken @bnjbvr
[codegen] reintroduce support for carry and borrow instructions in RI… (
8945b82 
#1005)

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.
arkpar pushed a commit to paritytech/wasmtime that referenced this pull request Mar 4, 2020
@ryzokuken @bnjbvr
[codegen] reintroduce support for carry and borrow instructions in RI… (
3418fb6 
bytecodealliance#1005)

Reintroduce support for iadd carry variants and isub borrow variants for
RISC ISAs which had been removed in
bytecodealliance/cranelift#961 and
bytecodealliance/cranelift#962 because of the lack
of a proper flags register in RISC architectures.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@bjorn3 bjorn3 bjorn3 left review comments

@bnjbvr bnjbvr bnjbvr approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ryzokuken @bjorn3 @bnjbvr