1520478 - Wasm compilers: split MIRType::Pointer uses into MIRType::{Pointer or RefOrNull}

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement)

Description

[Julian Seward [:jseward]]

Currently both wasm compilers use MIRType::Pointer to refer to objects
allocated both on the C++ heap and the JS heap, and in fact to pretty
much any flavour of "address of something".

With the introduction of GC support for wasm we need to be much clearer
about which pointers denote a GCable object and which don't. This bug
introduces a new type, MIRType::RefOrNull, to denote GCable pointers
in wasm-land, and converts most but not all existing MIRType::Pointer values
to MIRType::RefOrNull.

Comment 1

[Julian Seward [:jseward]]

Attachment: bug1520478-MIRType-RefOrNull-1.diff

Initial implementation, for perusal. It works well enough to survive

 JS_GC_ZEAL=2,100 \
   ../src/jit-test/jit_test.py -f ./dist/bin/js --args="--no-wasm-ion" wasm/gc

and

 ../src/jit-test/jit_test.py -f ./dist/bin/js wasm

for arm{32,64} and x86_{32,64}.

WIP patch; there are 6 commented uses of MIRType::Pointer that are probably
correct but are in need of further checking.

Comment 2

[Lars T Hansen [:lth]]

Comment on attachment 9036924 [details] [diff] [review]
bug1520478-MIRType-RefOrNull-1.diff

Review of attachment 9036924 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/wasm/WasmStubs.cpp
@@ +326,5 @@
>    ABIArgGenerator abi;
>    ABIArg arg;
>  
>    // arg 1: ExportArg*
> +  arg = abi.next(MIRType::Pointer); // FIXME recheck

I think this is right.

@@ +336,5 @@
>          argv);
>    }
>  
>    // Arg 2: TlsData*
> +  arg = abi.next(MIRType::Pointer); // FIXME recheck

I think this is right.

@@ +527,5 @@
>  
>    MIRTypeVector coerceArgTypes;
>    MOZ_ALWAYS_TRUE(coerceArgTypes.append(MIRType::Int32));
> +  MOZ_ALWAYS_TRUE(coerceArgTypes.append(MIRType::Pointer)); // FIXME recheck
> +  MOZ_ALWAYS_TRUE(coerceArgTypes.append(MIRType::Pointer)); // FIXME recheck

I think these are probably correct (the last should be descriptor and the one before that callee, right, and the int32 is argc?) but all three could use a short comment about what the arg is.

@@ +1087,3 @@
>              MOZ_CRASH("generating a jit exit for anyref NYI");
>            } else {
> +            MOZ_RELEASE_ASSERT(IsFloatingPointType(type));

Move back to MOZ_ASSERT before landing?

@@ +1210,2 @@
>    static const MIRType typeArray[] = {MIRType::Pointer,   // Instance*
> +   /* FIXME: is this correct? */      MIRType::Pointer,   // funcImportIndex

Looks right, but it freaks me out that below we store 32-bit values for funcImportIndex and argv if they are stack args, even if they are designated Pointer here.

@@ +1528,5 @@
>  
>      // Coercion calls use the following stack layout (sp grows to the left):
>      //   | args | padding | Value argv[1] | padding | exit Frame |
>      MIRTypeVector coerceArgTypes;
> +    MOZ_ALWAYS_TRUE(coerceArgTypes.append(MIRType::Pointer)); // FIXME recheck

Add brief comment re what this represents.

Comment 3

[Julian Seward [:jseward]]

Attachment: bug1520478-MIRType-RefOrNull-2.diff

This is the same as the previous iteration of the patch, but with the
FIXME comments removed, and somewhat better tested.

Comment 4

[Lars T Hansen [:lth]]

Comment on attachment 9042450 [details] [diff] [review]
bug1520478-MIRType-RefOrNull-2.diff

Review of attachment 9042450 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/wasm/WasmBaselineCompile.cpp
@@ +4056,2 @@
>          continue;
>        }

nit: Can you reverse the order of disjuncts here to preserve the old filtering logic?  I know it doesn't make sense but the logic is supposed to be "only stack slots" and then "only references".

::: js/src/wasm/WasmStubs.cpp
@@ +1006,5 @@
>      ScratchFloat32Scope fpscratch(masm);
>      masm.loadFloat32(src, fpscratch);
>      masm.storeFloat32(fpscratch, dst);
>    } else {
> +    MOZ_RELEASE_ASSERT(type == MIRType::Double);

nit: I would prefer it if you made this branch "else if (type == MIRType::Double)" and add a new else with MOZ_CRASH, this follows the style we use elsewhere more closely.

@@ +1102,3 @@
>              MOZ_CRASH("generating a jit exit for anyref NYI");
>            } else {
> +            MOZ_RELEASE_ASSERT(IsFloatingPointType(type));

nit: Same comment as above.  If you're going to have a release assert, make it a separate branch + MOZ_CRASH instead, if practical.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba0fe524ced9
Wasm compilers: split MIRType::Pointer uses into MIRType::{Pointer or RefOrNull}.  r=lhansen.

Comment 6

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/ba0fe524ced9