Bug 163246 – B3 needs a special WasmBoundsCheck Opcode

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Keith Miller]

In order to make WASM as fast as possible we need to give B3 a through understanding of how WASM works. In particular, B3 needs to understand the memory model of WASM and what optimizations it can make within those constraints. To this end we should add a WASMCheckBounds opcode. This opcode should take the following bits of information: WASMCheckBounds(Value* ptr, Reg sizeRegister, size_t guardedBytes). When emitting a load WASM will first emit a WASMBoundsCheck value passing WASMCheckBounds(WASMUserPtrValue, pinnedSizeRegister, max(0, MMapedMemoryBeyondMaxRequested - WASMLoadOpOffsetImmediate - sizeOfLoadOpcode)). So if a module had a memory max of 1MB and we mapped 1088 KB (64 KB extra) and a WASM I32.Load operation with an offset immediate of 1024 bytes we could emit a WASMCheckBounds(@ptr, %r13, 64 * KB - 1024 - 4).

[Filip Pizlo]

Couldn't help thinking about this more.

Seems like you need WASMCheckBounds to have one form where the check is inclusive and one where it isn't (i.e. < versus <=).  Some kind of enum that indicates whether it's Inclusive/BelowEqual or Exclusive/Below.  Please add printInternal functions for any new enums, particularly since you'll need to dump it as part of WASMCheckBoundsValue::dump().

Also, what are you planning on doing about representing this in Air?

You have these options:

1) Patch opcode, and a Air::Special subclass that does what you want.
2) A Custom opcode, and code in AirCustom.h|cpp that does what you want.
3) A normal opcode, and code in MacroAssembler that does what you want.

I think that (2) will be the most natural.  You'll want two Custom opcodes: WASMCheckBoundsBelow and WASMCheckBoundsBelowEqual, and they will basically be magical versions of jit.branch(Below or BelowEqual, ...).

[Keith Miller]

Created attachment 291378 [details]
Patch

[Filip Pizlo]

Comment on attachment 291378 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=291378&action=review

> Source/JavaScriptCore/ChangeLog:3
> +        B3 needs a special WASMBoundsCheck Opcode

You should probably rename the title.

> Source/JavaScriptCore/B3WasmBoundsCheckValue.cpp:50
> +        comma, "generator = ", RawPointer(m_generator.get()), ", sizeRegister = ", m_pinnedGPR,

I don't understand why you have a generator.

> Source/JavaScriptCore/B3WasmBoundsCheckValue.cpp:67
> +CCallHelpers::Jump WasmBoundsCheckValue::generate(Air::Inst& inst, CCallHelpers& jit, Air::GenerationContext& context)
> +{
> +    CCallHelpers::Jump outOfBounds = Air::Inst(Air::Branch64, this, Air::Arg::relCond(CCallHelpers::AboveOrEqual), inst.args[0], inst.args[1]).generate(jit, context);
> +
> +    context.latePaths.append(createSharedTask<Air::GenerationContext::LatePathFunction>(
> +        [=] (CCallHelpers& jit, Air::GenerationContext&) {
> +            outOfBounds.link(&jit);
> +            m_generator->run(jit, m_pinnedGPR, m_offset);
> +        }));
> +
> +    // We said we were not a terminal.
> +    return CCallHelpers::Jump();
> +}

This doesn't make sense.  Why is this here?

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:2497
> +                    append(Add64, Arg::bigImm(value->offset()), temp);

There is no such instruction.  You're going to have to come up with some other way of handling this case.

> Source/JavaScriptCore/b3/B3WasmBoundsCheckValue.h:66
> +    void setGenerator(RefPtr<WasmBoundsCheckGenerator> generator)
> +    {
> +        m_generator = generator;
> +    }
> +
> +    template<typename Functor>
> +    void setGenerator(const Functor& functor)
> +    {
> +        m_generator = createSharedTask<WasmBoundsCheckGeneratorFunction>(functor);
> +    }

This seems like totally unnecessary indirection.

> Source/JavaScriptCore/b3/air/AirCustom.h:310
> +        return static_cast<WasmBoundsCheckValue*>(inst.origin)->generate(inst, jit, context);

This should do the code generation directly.  You don't get anything by having a generator callback.

[Keith Miller]

Created attachment 291382 [details]
Patch

[Filip Pizlo]

Comment on attachment 291382 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=291382&action=review

> Source/JavaScriptCore/B3WasmBoundsCheckValue.cpp:62
> +            m_generator->run(jit, m_pinnedGPR, m_offset);

Hmmm.  I missed this part last time around.

If you want to keep the generator then I recommend inlining WasmBoundsCheckValue into WasmBoundsCheckCustom.

I don't think we want the generator, though.  Isn't it always going to be the same code?  I think it's better to just have B3 know what that code is!

[Filip Pizlo]

(In reply to comment #5)
> Comment on attachment 291382 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291382&action=review
> 
> > Source/JavaScriptCore/B3WasmBoundsCheckValue.cpp:62
> > +            m_generator->run(jit, m_pinnedGPR, m_offset);
> 
> Hmmm.  I missed this part last time around.
> 
> If you want to keep the generator then I recommend inlining
> WasmBoundsCheckValue into WasmBoundsCheckCustom.

*WasmBoundsCheckValue::generate into WasmBoundsCheckCustom::generate.

> 
> I don't think we want the generator, though.  Isn't it always going to be
> the same code?  I think it's better to just have B3 know what that code is!

[Keith Miller]

(In reply to comment #6)
> (In reply to comment #5)
> > Comment on attachment 291382 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=291382&action=review
> > 
> > > Source/JavaScriptCore/B3WasmBoundsCheckValue.cpp:62
> > > +            m_generator->run(jit, m_pinnedGPR, m_offset);
> > 
> > Hmmm.  I missed this part last time around.
> > 
> > If you want to keep the generator then I recommend inlining
> > WasmBoundsCheckValue into WasmBoundsCheckCustom.
> 
> *WasmBoundsCheckValue::generate into WasmBoundsCheckCustom::generate.
> 
> > 
> > I don't think we want the generator, though.  Isn't it always going to be
> > the same code?  I think it's better to just have B3 know what that code is!

Fair enough, I deleted WasmBoundsCheckValue::generate and inlined the old code into WasmBoundsCheckCustom::generate. I still think we want to have a generator at some point since I think it's better to minimize the amount of wasm specific code in B3. In the mean time I can delete the generator since it's trivial to restore later.

[Filip Pizlo]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > Comment on attachment 291382 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=291382&action=review
> > > 
> > > > Source/JavaScriptCore/B3WasmBoundsCheckValue.cpp:62
> > > > +            m_generator->run(jit, m_pinnedGPR, m_offset);
> > > 
> > > Hmmm.  I missed this part last time around.
> > > 
> > > If you want to keep the generator then I recommend inlining
> > > WasmBoundsCheckValue into WasmBoundsCheckCustom.
> > 
> > *WasmBoundsCheckValue::generate into WasmBoundsCheckCustom::generate.
> > 
> > > 
> > > I don't think we want the generator, though.  Isn't it always going to be
> > > the same code?  I think it's better to just have B3 know what that code is!
> 
> Fair enough, I deleted WasmBoundsCheckValue::generate and inlined the old
> code into WasmBoundsCheckCustom::generate. I still think we want to have a
> generator at some point since I think it's better to minimize the amount of
> wasm specific code in B3. In the mean time I can delete the generator since
> it's trivial to restore later.

What about Procedure::setWasmBoundsCheckGenerator(things)?

Patchpoints need many different generators within the same procedure.  This doesn't.

[Keith Miller]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (In reply to comment #5)
> > > > Comment on attachment 291382 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=291382&action=review
> > > > 
> > > > > Source/JavaScriptCore/B3WasmBoundsCheckValue.cpp:62
> > > > > +            m_generator->run(jit, m_pinnedGPR, m_offset);
> > > > 
> > > > Hmmm.  I missed this part last time around.
> > > > 
> > > > If you want to keep the generator then I recommend inlining
> > > > WasmBoundsCheckValue into WasmBoundsCheckCustom.
> > > 
> > > *WasmBoundsCheckValue::generate into WasmBoundsCheckCustom::generate.
> > > 
> > > > 
> > > > I don't think we want the generator, though.  Isn't it always going to be
> > > > the same code?  I think it's better to just have B3 know what that code is!
> > 
> > Fair enough, I deleted WasmBoundsCheckValue::generate and inlined the old
> > code into WasmBoundsCheckCustom::generate. I still think we want to have a
> > generator at some point since I think it's better to minimize the amount of
> > wasm specific code in B3. In the mean time I can delete the generator since
> > it's trivial to restore later.
> 
> What about Procedure::setWasmBoundsCheckGenerator(things)?
> 
> Patchpoints need many different generators within the same procedure.  This
> doesn't.

Yeah, I think I can get behind that such an API.

[Keith Miller]

Created attachment 291397 [details]
Patch

[Keith Miller]

Created attachment 291398 [details]
rebased

[Filip Pizlo]

Comment on attachment 291398 [details]
rebased

View in context: https://bugs.webkit.org/attachment.cgi?id=291398&action=review

> Source/JavaScriptCore/b3/air/AirOpcode.opcodes:893
> +# This is a special wasm opcode that branches to a trap handler. Like Patch this takes a special that
> +# must be the first operand.

Is this really true?  Only Patch should take a Special.  It would be weird if we changed that assumption.

> Source/JavaScriptCore/b3/testb3.cpp:15206
> +    RUN(testWasmBoundsCheck(10000));

Add a test for ridonculous offset.

[Keith Miller]

Created attachment 291413 [details]
Patch for landing

[Keith Miller]

(In reply to comment #12)
> Comment on attachment 291398 [details]
> rebased
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291398&action=review
> 
> > Source/JavaScriptCore/b3/air/AirOpcode.opcodes:893
> > +# This is a special wasm opcode that branches to a trap handler. Like Patch this takes a special that
> > +# must be the first operand.
> 

Whoops, I meant to change that comment. I changed it to:

# This is a special wasm opcode that branches to a trap handler. This uses the generator located to Air::Code
# to produce the side-exit code.

> Is this really true?  Only Patch should take a Special.  It would be weird
> if we changed that assumption.
> 
> > Source/JavaScriptCore/b3/testb3.cpp:15206
> > +    RUN(testWasmBoundsCheck(10000));
> 
> Add a test for ridonculous offset.

Done.

[Keith Miller]

Created attachment 291418 [details]
Patch

[Keith Miller]

Created attachment 291419 [details]
Patch for landing

[Keith Miller]

Created attachment 291433 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 291433 [details]
Patch for landing

Clearing flags on attachment: 291433

Committed r207266: <http://trac.webkit.org/changeset/207266>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.