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

RESOLVED FIXED in Firefox 50

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: hev, Assigned: hev)

Tracking

unspecified
mozilla50
Other
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(6 attachments, 1 obsolete attachment)

Assignee

Description

3 years ago
See Bug 1268024
Assignee

Updated

3 years ago
Blocks: wasm-mips

Updated

3 years ago
Attachment #8771838 - Flags: review?(luke) → review+
Priority: -- → P3

Comment 2

3 years ago
Pushed by r@hev.cc:
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
Assignee

Updated

3 years ago
Keywords: leave-open
Assignee

Comment 3

3 years ago
I had originally planned to push it with codegen.
Attachment #8772446 - Flags: review?(luke)

Updated

3 years ago
Attachment #8772446 - Flags: review?(luke) → review+
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.
Assignee

Comment 5

3 years ago
(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.
Assignee

Comment 8

3 years ago
Attachment #8772667 - Flags: review?(luke)
Assignee

Comment 9

3 years ago
Attachment #8772668 - Flags: review?(bbouvier)
Assignee

Comment 10

3 years ago
Fix WasmStore(Int64).
Attachment #8772667 - Attachment is obsolete: true
Attachment #8772667 - Flags: review?(luke)
Attachment #8772677 - Flags: review?(luke)

Updated

3 years ago
Attachment #8772666 - Flags: review?(luke) → review+

Updated

3 years ago
Attachment #8772677 - Flags: review?(luke) → review+
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)?
Attachment #8772665 - Flags: review?(bbouvier) → review+
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).
Attachment #8772668 - Flags: review?(bbouvier) → review+
Assignee

Comment 14

3 years ago
(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.
Assignee

Comment 15

3 years ago
(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

3 years ago
Pushed by r@hev.cc:
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
Assignee

Updated

3 years ago
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.