Closed Bug 1448277 Opened 2 years ago Closed Last year

Generate GVN-able references to indirect wasm globals

Categories

(Core :: JavaScript Engine: JIT, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox62 + wontfix
firefox63 + fixed

People

(Reporter: lth, Assigned: jseward)

References

Details

Attachments

(1 file, 3 obsolete files)

See the top of bug 1412238 comment 29 for context.  Indirect wasm globals load a pointer to the global's storage location from the instance's globals area, and then reads or writes through the pointer that was read.  The way this is currently implemented we always read the pointer from the globals area before an access to the global datum.  However, it would be better to generate a proper MIR node for this so that it can be GVN'd; with that, adjoining accesses to the global datum only need read the pointer from the globals area once.

(Ideally we get this done for FF61, and it's not a large job, but it won't be the end of the world if it slips to FF62.)
Blocks: 1457055
Priority: P1 → P2
Julian, assigning this to you because I think it will be valuable experience with Ion.  But you should probably not land anything until Benjamin has finished his work on reference-typed globals.
Assignee: lhansen → jseward
Consider the following module fragment:

  (global (export "g") (mut i32) (i32.const 0))
  (func (export "f") (result i32)
   (set_global 0 (i32.add (get_global 0) (i32.const 1)))
   (get_global 0)))

This generates the following code:

[Codegen] instruction WasmParameter
[Codegen] instruction WasmLoadGlobalVar
[Codegen] movq       0x40(%r14), %rcx
[Codegen] movl       0x0(%rcx), %eax
[Codegen] instruction AddI
[Codegen] addl       $1, %eax
[Codegen] instruction WasmStoreGlobalVar
[Codegen] movq       0x40(%r14), %rcx
[Codegen] movl       %eax, 0x0(%rcx)

The two movq instructions should be gvn'able I think.
I should note that Ion knows a thing or two about globals already, for example, it knows not to reload it for the return expression.
Attached patch Load-side implementation (WIP) (obsolete) — Splinter Review
Attached patch Initial implementation (WIP) (obsolete) — Splinter Review
Attachment #8984209 - Attachment is obsolete: true
Requesting feedback.  Also, there are a couple of things I am not sure about:

MWasmLoadGlobalVarPriv::valueHash() const
MWasmLoadGlobalVarPriv::congruentTo(const MDefinition* ins) const
  should these also consider |isConstant_| ?

class MWasmLoadGlobalCell
  is it necessary to implement valueHash(), congruentTo() or foldsTo() ?
Attachment #8984475 - Attachment is obsolete: true
Attachment #8986240 - Flags: feedback?(lhansen)
Comment on attachment 8986240 [details] [diff] [review]
bug1448277-WAG-GVNable-indirs-4.diff

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

Much good stuff here and direction clearly right, so f+.  We should ask one of the jit people to slide his eyes over the alias analysis when this is ready for review.

"Priv" is not a great suffix for the renamed instructions.  Since this is short for "Private" then let's at least use that, but can we do better?  Could you just discard the "Priv" part and either stick with "...Cell" or use "...Indirect" for the other one?

::: js/src/jit/CodeGenerator.cpp
@@ +7140,5 @@
>      emitWasmCallBase(ins->mir(), ins->needsBoundsCheck());
>  }
>  
> +static void
> +helper_LoadGlobal_VarPriv_or_Cell(MacroAssembler& masm,

As a matter of naming, static global functions like these are always initial-capital camel-case, and we never use underscores in the middle of names.  And "helper" is superflous.  So "LoadGlobalVarPrivOrCell".

But really this has nothing to do with globals at all, it's a generic "load a mir-typed value from an address to a register" helper.  The canonical way to do this is to use not const LDefintion* dst but to pass an AnyRegister and use ToAnyRegister in the caller.

So the appropriate name might be something as simple as LoadMachineValue or somesuch, though to be sure for the store case we'll need a write barrier for the pointer case that's not there yet.

@@ +7141,5 @@
>  }
>  
> +static void
> +helper_LoadGlobal_VarPriv_or_Cell(MacroAssembler& masm,
> +                                  MIRType type, bool pointerTypeAllowed,

pointerTypeAllowed seems redundant?  The caller that passes false asserts that the type is not pointer type.  I would remove it and just have a default here that crashes like you have now.

@@ +7209,5 @@
> +                                      addr, ins->output());
> +}
> +
> +static void
> +helper_StoreGlobal_VarPriv_or_Cell(MacroAssembler& masm,

Ditto "StoreGlobalVarPrivOrCell".  And this too is somewhat generic, and probably should take an AnyRegister source.

@@ +7275,5 @@
> +{
> +    MWasmLoadGlobalVarPriv* mir = ins->mir();
> +
> +    MIRType type = mir->type();
> +    MOZ_ASSERT(type == MIRType::Int64 || type == MIRType::Pointer);

Why a pointer case here?

::: js/src/jit/MIR.cpp
@@ +5489,5 @@
> +        // No globals of different type can alias.  See bug 1467415 comment 3.
> +        if (type() != def->toWasmStoreGlobalCell()->value()->type())
> +            return AliasType::NoAlias;
> +
> +        // Can we do better than this?  I'm not sure.  For now, give up.

If we know that at least one of the globals is defined within the module being compiled then they can't alias because exporting happens after importing; we cannot currently import anything we define.

Put another way, only if both cells originated from importing can they alias.  This is information we should be able to add to the MIR node and use here.

@@ +5495,5 @@
> +
> +    return AliasType::MayAlias;
> +}
> +
> +// FIXME jseward: should this also consider isConstant_ ?

Wouldn't that be implied by globalDataOffset_?

@@ +5504,5 @@
>      hash = addU32ToHash(hash, globalDataOffset_);
>      return hash;
>  }
>  
> +// FIXME jseward: should this also consider isConstant_ ?

I don't think that's necessary, since that would be implied by globalDataOffset_, I think.

::: js/src/jit/MIR.h
@@ +14881,5 @@
> +    INSTRUCTION_HEADER(WasmLoadGlobalCell)
> +    TRIVIAL_NEW_WRAPPERS
> +    NAMED_OPERANDS((0, cellPtr))
> +
> +    // FIXME jseward: is it necessary to implement any of these?

In principle we would want to CSE two back-to-back loads from the same mutable global variable.  I would guess that at least congruentTo would be required to make that work, and valueHash for hygiene / performance.  But foldsTo is used for simplification and there's no applicable simplification to these nodes, I think.

::: js/src/wasm/WasmIonCompile.cpp
@@ +1027,5 @@
> +            // load from that pointer.
> +            MOZ_RELEASE_ASSERT(!isConst);
> +            auto* cellPtr = MWasmLoadGlobalVarPriv::New(
> +                                alloc(), MIRType::Pointer, globalDataOffset,
> +                                isConst, tlsPointer_);

Why isConst here and not "true"?  And since we know isConst is false here because of the assert we should just pass false here if that's what we mean, as for the store.  But I expect you mean "true".

Note getAliasSet for this node checks isConstant_, so presumably it's meant to be true sometime.

@@ +1050,5 @@
> +            // Pull a pointer to the value out of TlsData::globalArea, then
> +            // store through that pointer.
> +            auto* cellPtr = MWasmLoadGlobalVarPriv::New(
> +                                alloc(), MIRType::Pointer, globalDataOffset,
> +                                /*isConst=*/false, tlsPointer_);

And why false here and not true?  In this case the value should never change.
Attachment #8986240 - Flags: feedback?(lhansen) → feedback+
Keeping an eye on this for 62.
Thanks for the feedback.  Herewith a new patch that addresses all review
comments.  I also re-checked the alias analysis and congruentTo() stuff in the
light of bug 1467415.

> "Priv" is not a great suffix for the renamed instructions.  Since this is
> short for "Private" then let's at least use that, but can we do better?
> Could you just discard the "Priv" part and either stick with "...Cell" or
> use "...Indirect" for the other one?

I dumped the Priv.  So we're back to MWasm{Load,Store}GlobalVar, with
MWasm{Load,Store}GlobalCell being the new instructions.

@@ +7275,5 @@
> +{
> +    MWasmLoadGlobalVarPriv* mir = ins->mir();
> +
> +    MIRType type = mir->type();
> +    MOZ_ASSERT(type == MIRType::Int64 || type == MIRType::Pointer);
>
> Why a pointer case here?

This is CodeGenerator::visitWasmLoadGlobalVarI64.  The reason why is the same
reason that CodeGenerator::visitWasmLoadGlobalVar accepts (implicitly) a
pointer case -- that it has to, in the case where the value is indirect
(in this case, a pointer to an Int64).

::: js/src/jit/MIR.h
@@ +14881,5 @@
> +    INSTRUCTION_HEADER(WasmLoadGlobalCell)
> +    TRIVIAL_NEW_WRAPPERS
> +    NAMED_OPERANDS((0, cellPtr))
> +
> +    // FIXME jseward: is it necessary to implement any of these?
>
> In principle we would want to CSE two back-to-back loads from the same
> mutable global variable.  I would guess that at least congruentTo would
> be required to make that work, and valueHash for hygiene / performance.
> But foldsTo is used for simplification and there's no applicable
> simplification to these nodes, I think.

Done.  This does indeed succeed in CSEing back to back loads of the same var.

I noticed that MWasmLoadGlobalVar::congruentTo doesn't compare operands (it
has one operand, the tls pointer).  Benjamin tells me this is safe because the
TLS pointer is unique per instance, hence a constant in any given compilation.
I added such a check anyway, out of paranoia, but can remove it again if you
prefer; but in that case can we at least add a comment?

(in MDefinition* loadGlobalVar(unsigned globalDataOffset, bool isConst, bool isIndirect, MIRType type))
::: js/src/wasm/WasmIonCompile.cpp
@@ +1027,5 @@
> +            // load from that pointer.
> +            MOZ_RELEASE_ASSERT(!isConst);
> +            auto* cellPtr = MWasmLoadGlobalVarPriv::New(
> +                                alloc(), MIRType::Pointer, globalDataOffset,
> +                                isConst, tlsPointer_);

I removed the assertion.  Now I'm not sure why I put it there at all.

> (in MWasmLoadGlobalCell::mightAlias)
> +        // Can we do better than this?  I'm not sure.  For now, give up.
>
> If we know that at least one of the globals is defined within the module
> being compiled then they can't alias because exporting happens after
> importing; we cannot currently import anything we define.

I didn't try to incorporate this refinement.  If you want it, can we do it in
a followup?
Attachment #8986240 - Attachment is obsolete: true
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #8)
> Keeping an eye on this for 62.

I have to say, I would vote against this going in 62.  Others may disagree.

This is a modest performance hop-up for Wasm code generation, in an area which
seems to be a bit tricky to get right (alias analysis) and for which I don't 
see a lot of tests.  I'd prefer to let it bake on trunk for a while, and 
be released in 63.
Flags: needinfo?(lhenry)
Attachment #8991624 - Flags: review?(lhansen)
That's absolutely fine with me! Thanks for letting me know.
I'll mark this wontfix for 62 then, and as always, if you disagree, please let me know.
Flags: needinfo?(lhenry)
Comment on attachment 8991624 [details] [diff] [review]
bug1448277-WAG-GVNable-indirs-5.diff

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

Note, you need to ensure that we have test cases that hit all the combinations of {direct,indirect} x {i32,i64}.

Nice commit comment, but the hedging ("I think they are correct") probably does not belong in a commit comment :)  Should be sufficient to observe that we believe that the alias analysis is not optimal because we know there are certain known-not-to-alias cases that are not discovered.  BTW a followup bug for that would be fine.

Fix the nits and this is good to go I think.

::: js/src/jit/CodeGenerator.cpp
@@ +7239,5 @@
> +{
> +    MWasmLoadGlobalVar* mir = ins->mir();
> +
> +    MIRType type = mir->type();
> +    MOZ_ASSERT(IsNumberType(type) || IsSimdType(type) || type == MIRType::Pointer);

Amusing pre-existing problem: IsNumberType includes Int64, yet we are really quite eager to ensure that Int64 is not being used here.

Frankly this assert could be removed since LoadPrimitiveValue does a better job of it anyway.

@@ +7252,5 @@
> +{
> +    MWasmLoadGlobalCell* mir = ins->mir();
> +
> +    MIRType type = mir->type();
> +    MOZ_ASSERT(IsNumberType(type) || IsSimdType(type));

Ditto.

@@ +7297,5 @@
>  {
>      MWasmStoreGlobalVar* mir = ins->mir();
>  
>      MIRType type = mir->value()->type();
>      MOZ_ASSERT(IsNumberType(type) || IsSimdType(type));

Ditto.

@@ +7311,5 @@
> +{
> +    MWasmStoreGlobalCell* mir = ins->mir();
> +
> +    MIRType type = mir->value()->type();
> +    MOZ_ASSERT(IsNumberType(type) || IsSimdType(type));

Ditto.

@@ +7325,5 @@
>  {
>      MWasmLoadGlobalVar* mir = ins->mir();
> +
> +    MIRType type = mir->type();
> +    MOZ_ASSERT(type == MIRType::Int64 || type == MIRType::Pointer);

As discussed on IRC, MIRType::Pointer should never be seen here, this node is only emitted when the type is known to be Int64.  (This observation also leads to some simplification below.)  What was the isIndirect case in the old code is resolved differently at higher levels now.  Loading a non-indirect i64 leads to this node; loading an indirect i64 leads to loadglobalvar (for the pointer) and loadglobalcelli64 (for the value).

::: js/src/jit/MIR.cpp
@@ +5489,5 @@
> +        // No globals of different type can alias.  See bug 1467415 comment 3.
> +        if (type() != def->toWasmStoreGlobalCell()->value()->type())
> +            return AliasType::NoAlias;
> +
> +        // Can we do better than this?  I'm not sure.  For now, give up.

We should note here that we can, see comments.  And you could simply copy over the comment from the code you're replacing with the reference to the "4th rule", I think.

@@ +5517,5 @@
> +    // equivalence of offsets implies equivalence of constness.
> +    bool sameOffsets = globalDataOffset_ == other->globalDataOffset_;
> +    MOZ_ASSERT_IF(sameOffsets, isConstant_ == other->isConstant_);
> +
> +    return sameOffsets && congruentIfOperandsEqual(other);

I don't think we want to check if the operands are equal here.  As you remarked elsewhere, this is always the TLS and there is just one TLS *value* but the *operands* may still in principle appear different here, depending on how deep the analysis of equality goes.  If we're unlucky we will find that two nodes that are congruent (because there is one TLS value) may appear to the JIT as non-congruent, and we may miss out on the optimization.  So I would remove the check but add a comment stating this fact.

::: js/src/wasm/WasmIonCompile.cpp
@@ +1032,5 @@
> +            // applies to the denoted value as a whole.
> +            auto* cellPtr = MWasmLoadGlobalVar::New(
> +                                alloc(), MIRType::Pointer, globalDataOffset,
> +                                /*isConst=*/true, tlsPointer_);
> +            curBlock_->add(cellPtr);

Nit/public service: the calls to New() here and below should be indented differently, more like the code you're replacing.  See https://wiki.mozilla.org/JavaScript:SpiderMonkey:Coding_Style#Indentation.
Attachment #8991624 - Flags: review?(lhansen) → review+
Pushed by jseward@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3430a73e5f1c
Generate GVN-able references to indirect wasm globals.  r=lth.
https://hg.mozilla.org/mozilla-central/rev/3430a73e5f1c
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.