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

RESOLVED FIXED in Firefox 50

Status

()

P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: hev, Assigned: hev)

Tracking

unspecified
mozilla50
Other
Linux
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(6 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
See Bug 1268024
(Assignee)

Updated

2 years ago
Blocks: 1284414
(Assignee)

Comment 1

2 years ago
Created attachment 8771838 [details] [diff] [review]
Part 1 - Implement SupportsUnalignedAccesses.
Attachment #8771838 - Flags: review?(luke)

Updated

2 years ago
Attachment #8771838 - Flags: review?(luke) → review+

Updated

2 years ago
Priority: -- → P3

Comment 2

2 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

2 years ago
Keywords: leave-open
(Assignee)

Comment 3

2 years ago
Created attachment 8772446 [details] [diff] [review]
Part 2 - Implement WasmBoundsCheck/Load/Store.

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

Updated

2 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

2 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 6

2 years ago
Created attachment 8772665 [details] [diff] [review]
Part 3 - Add a temp register ptrCopy to LWasmLoadI64.
Attachment #8772665 - Flags: review?(bbouvier)
(Assignee)

Comment 7

2 years ago
Created attachment 8772666 [details] [diff] [review]
Part 4 - Fix WasmBoundsCheck/Load/Store in Lowering.
Attachment #8772666 - Flags: review?(luke)
(Assignee)

Comment 8

2 years ago
Created attachment 8772667 [details] [diff] [review]
Part 5 - Implement WasmBoundsCheck/Load/Store in Codegen.
Attachment #8772667 - Flags: review?(luke)
(Assignee)

Comment 9

2 years ago
Created attachment 8772668 [details] [diff] [review]
Part 6 - Implement LoadI64 in Codegen.
Attachment #8772668 - Flags: review?(bbouvier)
(Assignee)

Comment 10

2 years ago
Created attachment 8772677 [details] [diff] [review]
Part 5 - Implement WasmBoundsCheck/Load/Store in Codegen.

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

Updated

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

Updated

2 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

2 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

2 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

2 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

2 years ago
Keywords: leave-open

Comment 17

2 years ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.