Implement reference types in compiler-llvm. #2154

nlewycky · 2021-03-01T19:55:03Z

No description provided.

Remove spectest ignores for reference types tests on llvm. Extend llvm ABIs to pass externref and funcref. Expose all libcalls through the Libcalls enum. Add support for all libcalls to llvm object_file.rs, even libcalls that we aren't using. Add missing no_mangle to libcalls. Change 'memory' to 'memory32' in libcalls in preparation for the memory64 proposal. Remove 'local' from 'wasmer_local_memory_copy' in libcalls to fit naming convention. Add mangling of externref and funcref for llvm-debug-dir. Mark 'wasmer_func_ref' readonly.

MarkMcCaskey

Other than a few small issues, if it's passing all the spectests then this looks good to me!

Feel free to leave feedback based on anything you learned implementing this. This actually turned out to be a shallower task than I originally anticipated, so it won't help as much with reviewing the major internal changes.

MarkMcCaskey · 2021-03-01T19:59:34Z

lib/compiler-llvm/src/abi/aarch64_systemv.rs

@@ -140,8 +140,7 @@ impl Abi for Aarch64SystemV {
                        Type::I32 | Type::F32 => 32,
                        Type::I64 | Type::F64 => 64,
                        Type::V128 => 128,
-                        Type::ExternRef => unimplemented!("externref in the llvm backend"),
-                        Type::FuncRef => unimplemented!("funcref in the llvm backend"),
+                        Type::ExternRef | Type::FuncRef => 64, /* pointer */


We may be able to use something in VMOffsets for this to help prevent breakage. I don't think it'll change any time soon, but something to think about

The presence of VMOffsets is optional in this function. The issue is that we don't have a VMContext when producing trampolines, so we don't have a VMOffset for it either.

Yeah, it's fine. Really not a big deal because we do this thing in other places in compiler-llvm where we don't use data from VMOffsets so things may get out of sync. I think it's fine to not worry about it for now and we can update compiler-llvm later if we decide that that's a high enough priority thing to do.

Really, a lot of VMOffsets stuff is static. In this case I don't know if we actually need a VMOffsets but, yeah, generally speaking getting the size of things doesn't require anything other than knowing the size of a pointer on the target architecture. Maybe that means it needs to be split into two pieces

MarkMcCaskey · 2021-03-01T20:00:21Z

lib/compiler-llvm/src/abi/x86_64_systemv.rs

-                }
-                Type::ExternRef => unimplemented!("externref in the llvm backend"),
-                Type::FuncRef => unimplemented!("funcref in the llvm backend"),
+                _ => panic!("should only be called to repack 32-bit values"),


Just some other bug you noticed?

On x86-64 this function is only called to cast 32-bit values, and should only be called to cast 32-bit values. I think this was copied out of the aarch64 code. I found this because I was auditing for places where ExternRef/FuncRef weren't handled and then investigated.

MarkMcCaskey · 2021-03-01T20:01:19Z

lib/compiler-llvm/src/object_file.rs

+    libcalls.insert("wasmer_f32_ceil".to_string(), LibCall::CeilF32);
+    libcalls.insert("wasmer_f64_ceil".to_string(), LibCall::CeilF64);
+    libcalls.insert("wasmer_f32_floor".to_string(), LibCall::FloorF32);
+    libcalls.insert("wasmer_f64_floor".to_string(), LibCall::FloorF64);
+    libcalls.insert("wasmer_f32_nearest".to_string(), LibCall::NearestF32);
+    libcalls.insert("wasmer_f64_nearest".to_string(), LibCall::NearestF64);
+    libcalls.insert("wasmer_f32_trunc".to_string(), LibCall::TruncF32);
+    libcalls.insert("wasmer_f64_trunc".to_string(), LibCall::TruncF64);


Are we using these?

MarkMcCaskey · 2021-03-01T20:02:30Z

lib/compiler-llvm/src/translator/code.rs

+                let is_null = self.builder.build_is_null(value, "");
+                let is_null = self
+                    .builder
+                    .build_int_z_extend(is_null, self.intrinsics.i32_ty, "");


Is this because LLVM doesn't use condition flags / special registers? I'm surprised it doesn't also have the concept of a bool type for this then

build_is_null returns an i1 (a one-bit integer, so a bool), but webassembly only has 32 and 64 bit integers. This instruction is specified as producing a 32-bit integer, so we extend it here.

MarkMcCaskey · 2021-03-01T20:03:55Z

lib/compiler-llvm/src/translator/code.rs

+            Operator::ElemDrop { segment } => {
+                let segment = self
+                    .intrinsics
+                    .i32_ty
+                    .const_int(segment as u64, false)
+                    .as_basic_value_enum();
+                self.builder.build_call(
+                    self.intrinsics.elem_drop,
+                    &[self.ctx.basic(), segment],
+                    "",
+                );
+            }


Interesting, singlepass was also missing this. It must be part of the Wasm spec now though because the ref types proposal assumes it's implemented and it's not part of the ref types proposal 🤔

MarkMcCaskey · 2021-03-01T20:04:46Z

lib/compiler-llvm/src/translator/intrinsics.rs

-            ty
-        ))),
+        Type::FuncRef => Ok(intrinsics.funcref_ty.ptr_type(AddressSpace::Generic)),
+        Type::ExternRef => Ok(intrinsics.funcref_ty.ptr_type(AddressSpace::Generic)),


Typo? This says ExternRef is a pointer to funcref_ty

Nope! intrinsics does not have a separate externref_ty. We could all the name and just make it equal to funcref_ty in intrisics.rs?

I think it may make sense to be more precise here. I'd be in favor of a reference_ty or anyref_ty or something to indicate it's an opaque pointer that we don't know / care about the specifics of, when that's the case like in table.set.

The main reason is just that we use funcref_ty semantically somewhere and it's also a bit confusing to read.

We already have an anyref_ty and in fact funcref_ty is defined as a pointer-to-anyref_ty!

What's an anyref_ty anyways? Is that the right name for what it is?

Yeah, I think that name might be confusing. funcref should be a pointer to a specific struct. I believe that's called a VMCallerCheckedAnyfunc I'd expect an anyref to be an opaque pointer. It's probably safe to assume that this opaque pointer at least requires alignment of 1 word, unsure if that really matters, but it should be a safe assumption.

MarkMcCaskey · 2021-03-01T20:04:53Z

lib/compiler-llvm/src/translator/intrinsics.rs

-            ty
-        ))),
+        Type::FuncRef => Ok(intrinsics.funcref_ty.as_basic_type_enum()),
+        Type::ExternRef => Ok(intrinsics.funcref_ty.as_basic_type_enum()),


MarkMcCaskey · 2021-03-01T20:05:55Z

lib/compiler-llvm/src/translator/intrinsics.rs

+                        ctx_ptr_ty.as_basic_type_enum(),
+                        i32_ty_basic,
+                        i32_ty_basic,
+                        funcref_ty.as_basic_type_enum(),


here as well, is funcref_ty the thing we're using for all reference types? Might be better to give it a different name when it's used in opaque contexts, as we do deref a funcref_ty in call indirect

MarkMcCaskey · 2021-03-01T20:06:59Z

lib/compiler-llvm/src/translator/intrinsics.rs

+            ),
+            func_ref: module.add_function(
+                "wasmer_func_ref",
+                funcref_ty.fn_type(&[i8_ptr_ty_basic, i32_ty_basic], false),


Does this not take a context pointer? I thought they all did because that's what the backends did by default

Yep that's a bug, thanks!

MarkMcCaskey · 2021-03-01T20:08:01Z

lib/compiler-llvm/src/translator/intrinsics.rs

+            elem_drop: module.add_function(
+                "wasmer_elem_drop",
+                void_ty.fn_type(&[i8_ptr_ty_basic, i32_ty_basic], false),
+                None,
+            ),
+            throw_trap: module.add_function(
+                "wasmer_raise_trap",
+                void_ty.fn_type(&[i32_ty_basic], false),
+                None,
+            ),
+


check these too

raise_trap is correct.

MarkMcCaskey · 2021-03-01T21:33:11Z

lib/compiler-llvm/src/translator/code.rs

+            Operator::RefNull { .. } => {
+                self.state.push1(self.intrinsics.funcref_ty.const_null());
+            }


This function takes a type which will tell you if it's a funcref or an externref

Good catch, thanks. Fixed!

nlewycky requested review from Hywan, jubianchi, losfair, MarkMcCaskey and syrusakbary as code owners March 1, 2021 19:55

nlewycky changed the base branch from master to feature/reference-types March 1, 2021 19:55

nlewycky removed request for syrusakbary, jubianchi, Hywan and losfair March 1, 2021 19:55

MarkMcCaskey approved these changes Mar 1, 2021

View reviewed changes

nlewycky added 2 commits March 1, 2021 12:17

Correct the type of vmctx on these functions.

7d9dd4d

Split funcref and externref into separate LLVM types, add anyref_ty.

3c6168d

MarkMcCaskey approved these changes Mar 1, 2021

View reviewed changes

RefNull may return any reference type.

40f40bb

nlewycky merged commit d13c128 into feature/reference-types Mar 1, 2021

bors bot deleted the feature/reference-types-llvm branch March 1, 2021 21:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement reference types in compiler-llvm. #2154

Implement reference types in compiler-llvm. #2154

nlewycky commented Mar 1, 2021

MarkMcCaskey left a comment

MarkMcCaskey Mar 1, 2021

nlewycky Mar 1, 2021

MarkMcCaskey Mar 1, 2021

MarkMcCaskey Mar 1, 2021

MarkMcCaskey Mar 1, 2021

nlewycky Mar 1, 2021

MarkMcCaskey Mar 1, 2021

nlewycky Mar 1, 2021

MarkMcCaskey Mar 1, 2021

nlewycky Mar 1, 2021

MarkMcCaskey Mar 1, 2021

MarkMcCaskey Mar 1, 2021

nlewycky Mar 1, 2021

MarkMcCaskey Mar 1, 2021

nlewycky Mar 1, 2021

MarkMcCaskey Mar 1, 2021

MarkMcCaskey Mar 1, 2021

MarkMcCaskey Mar 1, 2021

MarkMcCaskey Mar 1, 2021

nlewycky Mar 1, 2021

MarkMcCaskey Mar 1, 2021

nlewycky Mar 1, 2021

MarkMcCaskey Mar 1, 2021

nlewycky Mar 1, 2021

Implement reference types in compiler-llvm. #2154

Implement reference types in compiler-llvm. #2154

Conversation

nlewycky commented Mar 1, 2021

MarkMcCaskey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment