Add reference types to Cranelift by c27kwan · Pull Request #871 · bytecodealliance/cranelift

c27kwan · 2019-07-25T19:57:22Z

No description provided.

cranelift-codegen/meta/src/cdsl/types.rs

cranelift-simplejit/src/backend.rs

sunfishcode · 2019-07-26T13:58:31Z

Looks good! Other than the issue @philipc pointed out, this is in good shape!

cranelift-codegen/meta/src/shared/instructions.rs

bnjbvr

Looking great! First round of comments on the first commit, mostly nits / naming suggestions, and small changes to remove the ref-to-bool conversions that don't feel type safe. (I'll continue the review later today.)

cranelift-codegen/meta/src/cdsl/types.rs

cranelift-codegen/meta/src/cdsl/typevar.rs

cranelift-codegen/meta/src/shared/types.rs

cranelift-codegen/src/ir/instructions.rs

cranelift-codegen/src/ir/types.rs

bnjbvr · 2019-08-06T10:59:54Z

Also, wanted to point out that you can give attribution to several authors on Github commits with this feature, if you're in the mood for doing so.

bnjbvr

Second commit's feedback. I imagine the trapnf instruction is used thereafter.

cranelift-codegen/meta/src/shared/instructions.rs

cranelift-codegen/src/verifier/mod.rs

cranelift-simplejit/src/memory.rs

cranelift-simplejit/src/backend.rs

bnjbvr

Great work there! I think it is moving into the right direction. Many comments, but most of them are nits. I suspect some generated code is dead, we'll see.

I am still a bit unclear about what the new non-fatal trap does or allows; can you explain it a bit more, please? Thanks!

bnjbvr · 2019-08-06T13:14:39Z

cranelift-codegen/src/binemit/memorysink.rs

+
+    fn add_stackmap(&mut self, val_list: &[Value], func: &Function, isa: &dyn TargetIsa) {
+        let ofs = self.offset();
+        let sm = Stackmap::from_values(&val_list, func, isa);


nit: can you expand these two variable names to "offset" and "stack_map", please?

I'll expand sm to stackmap, but ofs is more consistent with the rest of the impl for CodeSink. We use ofs inside of reloc_ebb , reloc_external, and reloc_jt. In general, the variable names in this impl are very short.

cranelift-codegen/src/binemit/memorysink.rs

cranelift-codegen/src/binemit/stackmap.rs

cranelift-faerie/src/backend.rs

cranelift-filetests/src/test_savepoint.rs

cranelift-codegen/meta/src/isa/x86/recipes.rs

cranelift-codegen/meta/src/isa/x86/encodings.rs

bnjbvr · 2019-08-06T14:24:57Z

Ok, I looked at the last commit, which resolves some of the comments I've opened :) If possible, it'd be nice to play a bit with git rebase and include the changes from the last commit within the commit that introduced them. (The git-absorb addon could be useful to help you!)

Great work overall. Two open-ended questions:

do we want to support 32-bits references on 64-bits platforms, and conversely?
is there any additional work that's needed in Spidermonkey / Baldrdash to support ref types with the Cranelift backend for wasm?

Thanks!

c27kwan · 2019-08-06T18:09:53Z

@bnjbvr About trapnf: When writing the test files and mocking the trap interrupt that happens in function prologues, I was not able to use the trap instruction since it must always be a terminal. Presumably, we use a different kind of trap instruction that is non-terminal (e.g. trapif ) when we build the function prologue. After a discussion with @sunfishcode, it didn't seem right to only support traps that will terminate the program or conditional traps . So we added trapnf for general purpose traps that don't need to be a terminal in a block.

bnjbvr · 2019-08-07T09:18:47Z

@c27kwan Thanks for the explanation for trapnf, sounds good to me.

cranelift-faerie/src/backend.rs

cranelift-codegen/src/binemit/stackmap.rs

filetests/safepoint/basic.clif

cranelift-codegen/meta/src/shared/instructions.rs

bnjbvr

I've skimmed this again, and this looks in great shape for landing! (One minor comment, but we could take care of it after landing.)
Congratulations and great work here, thanks!

cranelift-codegen/meta/src/shared/instructions.rs

-Add resumable_trap, safepoint, isnull, and null instructions -Add Stackmap struct and StackmapSink trait Co-authored-by: Mir Ahmed <mirahmed753@gmail.com> Co-authored-by: Dan Gohman <sunfish@mozilla.com>

sunfishcode

This looks good to me to!

bjorn3 suggested changes Jul 25, 2019

View reviewed changes

cranelift-codegen/meta/src/cdsl/types.rs Show resolved Hide resolved

c27kwan mentioned this pull request Jul 26, 2019

Stackmaps #781

Closed

philipc reviewed Jul 26, 2019

View reviewed changes

cranelift-simplejit/src/backend.rs Outdated Show resolved Hide resolved

bjorn3 reviewed Jul 26, 2019

View reviewed changes

cranelift-codegen/meta/src/shared/instructions.rs Outdated Show resolved Hide resolved

bnjbvr self-requested a review August 6, 2019 09:28

bnjbvr reviewed Aug 6, 2019

View reviewed changes

bnjbvr suggested changes Aug 6, 2019

View reviewed changes

jamatthews mentioned this pull request Aug 7, 2019

Investigate using cranelift to implement a Ruby compiler artichoke/artichoke#87

Open

philipc reviewed Aug 7, 2019

View reviewed changes

cranelift-faerie/src/backend.rs Show resolved Hide resolved

c27kwan force-pushed the savepoint branch 2 times, most recently from ee60042 to f288568 Compare August 14, 2019 22:04

lars-t-hansen reviewed Aug 15, 2019

View reviewed changes

cranelift-codegen/src/binemit/stackmap.rs Outdated Show resolved Hide resolved

lars-t-hansen reviewed Aug 15, 2019

View reviewed changes

filetests/safepoint/basic.clif Show resolved Hide resolved

acfoltzer mentioned this pull request Aug 15, 2019

Plans for reference-types proposal? bytecodealliance/lucet#265

Open

sunfishcode reviewed Aug 15, 2019

View reviewed changes

cranelift-codegen/meta/src/shared/instructions.rs Outdated Show resolved Hide resolved

c27kwan force-pushed the savepoint branch from f288568 to 5ff98d6 Compare August 15, 2019 23:59

bnjbvr reviewed Aug 16, 2019

View reviewed changes

cranelift-codegen/meta/src/shared/instructions.rs Outdated Show resolved Hide resolved

Add reference types R32 and R64

0ee24d9

-Add resumable_trap, safepoint, isnull, and null instructions -Add Stackmap struct and StackmapSink trait Co-authored-by: Mir Ahmed <mirahmed753@gmail.com> Co-authored-by: Dan Gohman <sunfish@mozilla.com>

c27kwan force-pushed the savepoint branch from 5ff98d6 to 0ee24d9 Compare August 16, 2019 17:08

sunfishcode approved these changes Aug 16, 2019

View reviewed changes

sunfishcode merged commit 39f3fd0 into bytecodealliance:master Aug 16, 2019

tschneidereit mentioned this pull request Aug 19, 2019

Support Reference Types bytecodealliance/wasmtime#285

Closed

Conversation

c27kwan commented Jul 25, 2019

Uh oh!

Uh oh!

Uh oh!

sunfishcode commented Jul 26, 2019

Uh oh!

Uh oh!

bnjbvr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bnjbvr commented Aug 6, 2019

Uh oh!

bnjbvr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bnjbvr left a comment

Choose a reason for hiding this comment

Uh oh!

bnjbvr Aug 6, 2019

Choose a reason for hiding this comment

Uh oh!

c27kwan Aug 7, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bnjbvr commented Aug 6, 2019

Uh oh!

c27kwan commented Aug 6, 2019

Uh oh!

bnjbvr commented Aug 7, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bnjbvr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sunfishcode left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants