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

I128 #795

Merged
merged 27 commits into from
Sep 7, 2019
Merged

I128 #795

merged 27 commits into from
Sep 7, 2019

Conversation

bjorn3
Copy link
Contributor

@bjorn3 bjorn3 commented Jun 12, 2019

cc #354

This adds a i128 type. While only iconcat and isplit support them, this makes it easier cg_clif to handle 128 bit integers, as the low and high bits are bundled together. cg_clif would need a bigger change to special case i128 as not consisting of just one Value. It also makes it easier to inspect the ir generated by cg_clif, as there is no need to think about which two values together represent the i128.

You can create an i128 value by using iconcat with two i64 values: first the lsb, then the msb (on little endian)

TODO

  • Fix isplit i128 -> i64,i64

@bjorn3
Copy link
Contributor Author

bjorn3 commented Jun 13, 2019

The test fails with:

function u0:0(i64 [%rdi], i64 [%rsi]) -> i64 [%rax], i64 [%rdx], i64 [%rcx], i64 [0] fast {
                                ebb0(v0: i64 [%rdi], v1: i64 [%rsi]):
[RexOp1pu_id#b8,%rax]               v2 = iconst.i64 42
[RexOp1pu_id#b8,%rcx]               v3 = iconst.i64 0
[RexOp1rmov#8089]                   regmove v3, %rcx -> %rdx
[RexOp1rmov#8089]                   regmove v0, %rdi -> %rcx
[Op1ret#c3]                         return v2, v3, v0, v1
;~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
; error: inst3: ABI expects v1 at stack offset 0, got %rsi

}

Can somebody help me with fixing it?

@sunfishcode
Copy link
Member

The immediate problem is that the x86 backend only supports 3 integer values to be returned in registers, and after splitting the i128 into two, the function would require 4 integer return registers.

We'll need to convert functions with return types like that to use out parameters. One way to do that is to require frontends to lower such code to using out parameters instead -- and ideally add helper routines in cranelift-frontend to make it easy for frontends to do this. Another would be to teach the legalization phase in cranelift-codegen to convert such return types into out parameters.

I have a mild preference for the former, to keep the code in cranelift-codegen as simple as possible, though either would work.

@sunfishcode
Copy link
Member

For the record, 3 integer types is more than is specified in the System V ABI, but is consistent with LLVM.

@bjorn3
Copy link
Contributor Author

bjorn3 commented Jun 16, 2019

The immediate problem is that the x86 backend only supports 3 integer values to be returned in registers, and after splitting the i128 into two, the function would require 4 integer return registers.

That explains it. Using an out param when a value returned ByValPair consists of two i128 values should workaround this problem for cg_clif then.

One way to do that is to require frontends to lower such code to using out parameters instead -- and ideally add helper routines in cranelift-frontend to make it easy for frontends to do this.

When this option is chosen, it would be nice to add a verifier error for more than three ret values on x86.

I want to add some legalizations for loading and storing i128 before this is merged.

@bjorn3
Copy link
Contributor Author

bjorn3 commented Jul 7, 2019

I got the cg_clif side working: rust-lang/rustc_codegen_cranelift#627

@sunfishcode
Copy link
Member

How do you expect to implement things like add/sub/mul on i128 types?

@bjorn3
Copy link
Contributor Author

bjorn3 commented Jul 9, 2019

add and sub are already legalized to 64bit ops by the same code legalizing the 64bit versions to 32bit on i686. There rest of the math instructions are codegened to compiler_builtins calls by cg_clif.

@bjorn3
Copy link
Contributor Author

bjorn3 commented Jul 19, 2019

The linux build timed out during cache unpacking. The macOS build did succeed however.

@bjorn3
Copy link
Contributor Author

bjorn3 commented Jul 24, 2019

Travis is failing during cache (un)packing again.

@sunfishcode
Copy link
Member

Eek, I don't know what's going on with Travis here. If anyone has any ideas of what we should do here, please share :-}.

@ethanhs
Copy link

ethanhs commented Jul 25, 2019

I've run into issues with Travis timing out before, and its been doing it more lately. In general going to Travis and clicking restart build helps, have you tried that? (If that doesn't work, then maybe it is timing out downloading caches?)

@sunfishcode
Copy link
Member

I've now clicked "restart build".

@bjorn3
Copy link
Contributor Author

bjorn3 commented Jul 25, 2019

Travis is green

@bjorn3
Copy link
Contributor Author

bjorn3 commented Aug 2, 2019

@bnjbvr could you review this?

@bnjbvr
Copy link
Member

bnjbvr commented Aug 5, 2019

Yes, I could get to it; unfortunately I'm a bit in a review debt situation, so there might be many other things before I can come back to this.

@bnjbvr bnjbvr self-requested a review August 5, 2019 13:14
@bjorn3
Copy link
Contributor Author

bjorn3 commented Aug 5, 2019

Thanks!

@bnjbvr
Copy link
Member

bnjbvr commented Aug 21, 2019

Sorry, I realistically don't have the time these days to review this, deferring to Dan.

@bnjbvr bnjbvr requested review from sunfishcode and removed request for bnjbvr August 21, 2019 16:19
@bjorn3
Copy link
Contributor Author

bjorn3 commented Sep 7, 2019

Rebased

Copy link
Member

@sunfishcode sunfishcode left a comment

Choose a reason for hiding this comment

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

Cool, thanks for seeing this all the way through!

@sunfishcode sunfishcode merged commit b117cfd into bytecodealliance:master Sep 7, 2019
@bjorn3 bjorn3 deleted the i128 branch September 7, 2019 17:50
@bjorn3
Copy link
Contributor Author

bjorn3 commented Sep 7, 2019

I can finally move cg_clif back to upstream Cranelift 🎉

@bjorn3
Copy link
Contributor Author

bjorn3 commented Sep 8, 2019

@bjorn3 bjorn3 mentioned this pull request Sep 28, 2019
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.

None yet

5 participants