Closed Bug 1287349 Opened 4 years ago Closed 4 years ago

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

Categories

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

Other
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: hev, Assigned: hev)

References

Details

Attachments

(6 files, 1 obsolete file)

See Bug 1268024
Blocks: wasm-mips
Attachment #8771838 - Flags: review?(luke)
Attachment #8771838 - Flags: review?(luke) → review+
Priority: -- → P3
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
Keywords: leave-open
I had originally planned to push it with codegen.
Attachment #8772446 - Flags: review?(luke)
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.
(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.
Attachment #8772665 - Flags: review?(bbouvier)
Attachment #8772667 - Flags: review?(luke)
Attachment #8772668 - Flags: review?(bbouvier)
Fix WasmStore(Int64).
Attachment #8772667 - Attachment is obsolete: true
Attachment #8772667 - Flags: review?(luke)
Attachment #8772677 - Flags: review?(luke)
Attachment #8772666 - Flags: review?(luke) → review+
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+
(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.
(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
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
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.