1287349 - OdinMonkey: MIPS: Implement WasmBoundsCheck/Load/Store and semantics

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P3)

Description

[hev [:hev]]

See Bug 1268024

Comment 1

[hev [:hev]]

Attachment: Part 1 - Implement SupportsUnalignedAccesses.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3864de2e7947
IonMonkey: MIPS: Implement SupportsUnalignedAccesses. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/063faa868331
OdinMonkey: MIPS: Implement WasmBoundsCheck/Load/Store and semantics. r=luke

Comment 3

[hev [:hev]]

Attachment: Part 2 - Implement WasmBoundsCheck/Load/Store.

I had originally planned to push it with codegen.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8772446 [details] [diff] [review]
Part 2 - Implement WasmBoundsCheck/Load/Store.

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

Sorry for the drive-by review; I noticed this landed without review. Do you have a patch for the codegen too?

::: js/src/jit/mips-shared/Lowering-mips-shared.cpp
@@ +317,5 @@
> +      case Scalar::Uint32:
> +        valueAlloc = useRegisterOrConstantAtStart(value);
> +        break;
> +      case Scalar::Int64:
> +        // No way to encode an int64-to-memory move on x64.

This comment is at least wrong in the sense that it mentions x64. Can MIPS encode an int64 to memory move? If so, please delete the comment; otherwise, please update it.

Comment 5

[hev [:hev]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #4)
> Comment on attachment 8772446 [details] [diff] [review]
> Part 2 - Implement WasmBoundsCheck/Load/Store.
> 
> Review of attachment 8772446 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the drive-by review; I noticed this landed without review. Do you
> have a patch for the codegen too?
> 
> ::: js/src/jit/mips-shared/Lowering-mips-shared.cpp
> @@ +317,5 @@
> > +      case Scalar::Uint32:
> > +        valueAlloc = useRegisterOrConstantAtStart(value);
> > +        break;
> > +      case Scalar::Int64:
> > +        // No way to encode an int64-to-memory move on x64.
> 
> This comment is at least wrong in the sense that it mentions x64. Can MIPS
> encode an int64 to memory move? If so, please delete the comment; otherwise,
> please update it.

Thank you for your notice. It's my fault. :( I will upload fix patch and codegen.

Comment 6

[hev [:hev]]

Attachment: Part 3 - Add a temp register ptrCopy to LWasmLoadI64.

Comment 7

[hev [:hev]]

Attachment: Part 4 - Fix WasmBoundsCheck/Load/Store in Lowering.

Comment 8

[hev [:hev]]

Attachment: Part 5 - Implement WasmBoundsCheck/Load/Store in Codegen.

Comment 9

[hev [:hev]]

Attachment: Part 6 - Implement LoadI64 in Codegen.

Comment 10

[hev [:hev]]

Attachment: Part 5 - Implement WasmBoundsCheck/Load/Store in Codegen.

Fix WasmStore(Int64).

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3864de2e7947
https://hg.mozilla.org/mozilla-central/rev/063faa868331

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8772665 [details] [diff] [review]
Part 3 - Add a temp register ptrCopy to LWasmLoadI64.

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

Thanks!

::: js/src/jit/shared/LIR-shared.h
@@ +7747,5 @@
> +        setTemp(0, LDefinition::BogusTemp());
> +    }
> +
> +    const LDefinition* ptrCopy() {
> +        return Base::getTemp(0);

Can it just be getTemp(0)?

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8772668 [details] [diff] [review]
Part 6 - Implement LoadI64 in Codegen.

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

Looks good. I tend to think more and more that the WasmBoundsCheck should return the checked base + offset... (not to be done in this bug, as it's a multi platform issue).

Comment 14

[hev [:hev]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #12)
> Comment on attachment 8772665 [details] [diff] [review]
> Part 3 - Add a temp register ptrCopy to LWasmLoadI64.
> 
> Review of attachment 8772665 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks!
> 
> ::: js/src/jit/shared/LIR-shared.h
> @@ +7747,5 @@
> > +        setTemp(0, LDefinition::BogusTemp());
> > +    }
> > +
> > +    const LDefinition* ptrCopy() {
> > +        return Base::getTemp(0);
> 
> Can it just be getTemp(0)?

Yes, Although this function is used by WasmLoadI64 for MIPS64 only, This is done for code consistency and clarity.

Comment 15

[hev [:hev]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #13)
> Comment on attachment 8772668 [details] [diff] [review]
> Part 6 - Implement LoadI64 in Codegen.
> 
> Review of attachment 8772668 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. I tend to think more and more that the WasmBoundsCheck should
> return the checked base + offset... (not to be done in this bug, as it's a
> multi platform issue).

I think this is a good idea that forward the checked base + offset to WasmLoad or others from WasmBoundsCheck. :D

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/77fea9f0a2a7
wasm: Add a temp register ptrCopy to LWasmLoadI64. r=bbouvier
https://hg.mozilla.org/integration/mozilla-inbound/rev/81e2a6e05087
wasm: MIPS: Fix WasmBoundsCheck/Load/Store in Lowering. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/596b6c9838ec
wasm: MIPS: Implement WasmBoundsCheck/Load/Store in Codegen. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/301e457cab3f
wasm: MIPS64: Implement LoadI64 in Codegen. r=bbouvier

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/77fea9f0a2a7
https://hg.mozilla.org/mozilla-central/rev/81e2a6e05087
https://hg.mozilla.org/mozilla-central/rev/596b6c9838ec
https://hg.mozilla.org/mozilla-central/rev/301e457cab3f