Closed Bug 1421531 Opened 2 years ago Closed 2 years ago

mips64 wasm baseline

Categories

(Core :: JavaScript Engine: JIT, enhancement)

58 Branch
enhancement
Not set

Tracking

()

RESOLVED DUPLICATE of bug 1284414

People

(Reporter: yuyin-hf, Unassigned)

Details

Attachments

(3 files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/604.3.5 (KHTML, like Gecko) Version/11.0.1 Safari/604.3.5

Steps to reproduce:

I have did some work about support wasm baseline on mips64 platform some days ago, yesterday I test the code and all the jit-test pass. But when I just try to push
 it to upstream today, I find there are lots of change in WasmBaselineCompiler.cpp, I also find the bug - 1420838, it looks like there will be be lots of change about wasm baseline in a few days, so it is better to wait until these change check in and then merge my code?

but I still extracted two patches, one is fix some wrong functions implementation, another is help functions that wasm baseline needed. I think it is ok to push them now?

thank you.
Attachment #8932745 - Flags: review?(lhansen)
Attachment #8932746 - Flags: review?(lhansen)
Component: Untriaged → JavaScript Engine: JIT
Product: Firefox → Core
Attachment #8932745 - Flags: review?(lhansen) → review+
Comment on attachment 8932746 [details] [diff] [review]
0002-MIPS64-Add-functions-that-wasm-baseline-needed.patch

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

I've only looked very lightly at the MIPS code itself, not tried to make sure it's correct.  So this is more about the MacroAssembler API.

It doesn't make much sense (I think) to introduce wasmLoad and wasmStore and so on here if you're not going to be using them from CodeGenerator-mips-shared.cpp.  There's every reason for the wasm compilers to share this code if possible, as they do on ARM.

::: js/src/jit/MacroAssembler.h
@@ +1471,5 @@
>          DEFINED_ON(arm);
>  
> +    void wasmLoad(const wasm::MemoryAccessDesc& access, Register memoryBase, Register ptr,
> +                  AnyRegister output)
> +        DEFINED_ON(mips64);

See comments on MacroAssembler-mips64.cpp.  I would much prefer that you use the same API as ARM here, and then you can just add 'mips64' to the ARM clauses directly above, at least for some of these functions (as many as possible).

::: js/src/jit/mips64/CodeGenerator-mips64.cpp
@@ +667,5 @@
>  
> +        // Reset sixth bit of fcsr, flag = invalid operation.
> +        masm.as_cfc1(SecondScratchReg, Assembler::FCSR);
> +        masm.ma_and(SecondScratchReg, SecondScratchReg, Imm32(0xffffffbf));
> +        masm.as_ctc1(SecondScratchReg, Assembler::FCSR);

Since this cliche is used many places you might want to factor it into a method in the platform masm.  (Only a suggestion; it's OK with me if you leave it like it is, too.)

::: js/src/jit/mips64/MacroAssembler-mips64.cpp
@@ +2582,5 @@
> +                         Register index, AnyRegister out)
> +{
> +    memoryBarrier(access.barrierBefore());
> +    Register address = index;
> +    as_daddu(address, base, index); // do not change base(s7)

This is confusing, since 'index' is an actual clobberable temp here, what is known as 'ptrScratch' in the ARM code.

I really would prefer that you follow the ARM API here, even though that API seems a little silly (ptr and ptrScratch as two arguments, yet they must be the same register); that way, we maintain better coherency between the platforms.

Also see comments on MacroAssembler.h.

@@ +2642,5 @@
> +                            Register index, Register64 out)
> +{
> +    memoryBarrier(access.barrierBefore());
> +    Register address = index;
> +    as_daddu(address, base, index);

Ditto.

@@ +2731,5 @@
> +                          Register base, Register index)
> +{
> +    memoryBarrier(access.barrierBefore());
> +    Register address = index;
> +    as_daddu(address, base, index);

Ditto.

@@ +2785,5 @@
> +                             Register base, Register index)
> +{
> +    memoryBarrier(access.barrierBefore());
> +    Register address = index;
> +    as_daddu(address, base, index);

Ditto.
Attachment #8932746 - Flags: review?(lhansen)
(In reply to yuyin from comment #0)
> 
> I have did some work about support wasm baseline on mips64 platform some
> days ago, yesterday I test the code and all the jit-test pass. But when I
> just try to push it to upstream today, I find there are lots of change in
> WasmBaselineCompiler.cpp, I also find the bug - 1420838, it looks like there
> will be be lots of change about wasm baseline in a few days, so it is better
> to wait until these change check in and then merge my code?

Yes.  The main bug for this cleanup is bug 1336027, or really, all the bugs hanging off it.

> but I still extracted two patches, one is fix some wrong functions
> implementation, another is help functions that wasm baseline needed. I think
> it is ok to push them now?

I think you should wait on these as well.  And you should try to use wasmLoad() and wasmStore() for both ion and baseline, as the ARM port does; it's not good to have two code paths there if you can avoid it.
Hi,

Sorry for the late replay. These patches, as they are now, are breaking the mips32 build.
We thought of postponing any patches sent from our side until the wasm baseline code stabilizes from bug 1336027. The plan was to send the MIPS32 support first and then the MIPS64 on top of it. We are trying to bring up wasm and wasm baseline up to latest inbound changes like in Bug 1420838. Given that you have a working MIPS64 implementation we'll try to send our patches as soon as possible in order to converge our two implementations at least in mips-shared code. For example we chose the ARM's approach for wasmLoad and wasmStore API.  Is this issue a good place for that or should we use Bug 1284414? 

Off-topic question: Is the Loongson you are using NAN2008 capable?
Flags: needinfo?(yuyin-hf)
ok, thank you, I will wait.

(In reply to Lars T Hansen [:lth] from comment #4)

> I think you should wait on these as well.  And you should try to use
> wasmLoad() and wasmStore() for both ion and baseline, as the ARM port does;
> it's not good to have two code paths there if you can avoid it.
Flags: needinfo?(yuyin-hf)

(In reply to Dragan Mladjenovic from comment #5)
> Hi,
> 
> Sorry for the late replay. These patches, as they are now, are breaking the
> mips32 build.

really sorry, I do not have a 32-bit os now, I though this code would not afftect mips32, obviously I miss something. I will try to build on both 64-bit/32-bit next time.


> We thought of postponing any patches sent from our side until the wasm
> baseline code stabilizes from bug 1336027. The plan was to send the MIPS32
> support first and then the MIPS64 on top of it. We are trying to bring up
> wasm and wasm baseline up to latest inbound changes like in Bug 1420838.
> Given that you have a working MIPS64 implementation we'll try to send our
> patches as soon as possible in order to converge our two implementations at
> least in mips-shared code. For example we chose the ARM's approach for
> wasmLoad and wasmStore API. 

ok, thank you. I will wait until mips32 have done.

> Is this issue a good place for that or should
> we use Bug 1284414? 

oh, there was a bug already, I will close this one, and we can use that bug?

> 
> Off-topic question: Is the Loongson you are using NAN2008 capable?
no, for now same with r2, only r6 using NAN2008? I am just want to consult you how to solve this? looks like V8 just leave it alone?
Status: UNCONFIRMED → RESOLVED
Closed: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: wasm-mips
> > Off-topic question: Is the Loongson you are using NAN2008 capable?
> no, for now same with r2, only r6 using NAN2008? I am just want to consult
> you how to solve this? looks like V8 just leave it alone?

I said I had all jit-test pass, that is not true, some tests for example wasm/spec/f32.wast.js which have to do with signaling/quiet bit of nan. I had comment out this tests before.  

do you have any suggestion about mipsr2 & loongson which do not implement ieee754-2008? thank you.
Keywords: checkin-needed
Keywords: checkin-needed
(In reply to yuyin from comment #9)
> > > Off-topic question: Is the Loongson you are using NAN2008 capable?
> > no, for now same with r2, only r6 using NAN2008? I am just want to consult
> > you how to solve this? looks like V8 just leave it alone?
> 
> I said I had all jit-test pass, that is not true, some tests for example
> wasm/spec/f32.wast.js which have to do with signaling/quiet bit of nan. I
> had comment out this tests before.  
> 
> do you have any suggestion about mipsr2 & loongson which do not implement
> ieee754-2008? thank you.

Hi,

Thank you for your response. Currently only for mips32 we are already nonconforming to wasm specification because of lack of hardware support for 64-bit atomics, so we might just put the parts of wasm test that relay on exact NAN encoding behind some js shell flag which would disable them on non NAN2008 build and call it a day. Before that I will tray to add some form of NAN value canonization at least on non baseline wasm backend. Currently NAN encoding is observable on js <-> wasm ?, wasm <-> wasm calls, floating point memory stores, floating point equality checks, reinterpreting floating point value as integer. If I can come up with reasonably clean and noninvasive solution I will post it up. There is also an "issue" with js engine. The code doesn't seem to relay on exact NAN encoding for time being, but we should probably change the GenericNAN constant so it would use different encoding on non NAN 2008 MIPS targets?
Flags: needinfo?(lhansen)
I haven't done much with NaN canonicalization myself but I know Benjamin has, so let's ask him what he thinks about this.

(I believe Wasm specifies the encoding of signaling vs quiet NaNs but that otherwise there should be some flexibility.)
Flags: needinfo?(lhansen) → needinfo?(bbouvier)
Note that the specification says that propagating NaNs (in instructions) is permitted but not required [1]. That being said, I think all NaN patterns are allowed; these patterns shouldn't be modified as long as the values don't go through an instruction. So if I return NaN:0x123 from a function, another function should observe NaN:0x123 if it calls the former.

> Before that I will tray to add some form of NAN value canonization at least on non baseline wasm backend.

I think that would be contrary to the wasm specification, which allows any NaN custom payload to show up. Note that when calling out to JS, or when calling from JS, double values are already canonicalized. (I think if a wasm writes a custom NaN payload into memory and then it's read from JS as uint8, we could retrieve the pattern; but that's a JS issue too)

> The code doesn't seem to relay on exact NAN encoding for time being

There's just one constraint related to NaN-boxing (the NaN has to be acknowledged as such even when considered as a JS::Value, see [2]).

[1] https://github.com/WebAssembly/design/blob/master/Semantics.md#floating-point-operators
[2] https://searchfox.org/mozilla-central/source/js/public/Value.h#240-241
Flags: needinfo?(bbouvier)
(In reply to Benjamin Bouvier [:bbouvier] from comment #12)
> Note that the specification says that propagating NaNs (in instructions) is
> permitted but not required [1]. That being said, I think all NaN patterns
> are allowed; these patterns shouldn't be modified as long as the values
> don't go through an instruction. So if I return NaN:0x123 from a function,
> another function should observe NaN:0x123 if it calls the former.
> 
> > Before that I will tray to add some form of NAN value canonization at least on non baseline wasm backend.
> 
> I think that would be contrary to the wasm specification, which allows any
> NaN custom payload to show up. Note that when calling out to JS, or when
> calling from JS, double values are already canonicalized. (I think if a wasm
> writes a custom NaN payload into memory and then it's read from JS as uint8,
> we could retrieve the pattern; but that's a JS issue too)
> 
> > The code doesn't seem to relay on exact NAN encoding for time being
> 
> There's just one constraint related to NaN-boxing (the NaN has to be
> acknowledged as such even when considered as a JS::Value, see [2]).
> 
> [1]
> https://github.com/WebAssembly/design/blob/master/Semantics.md#floating-
> point-operators
> [2] https://searchfox.org/mozilla-central/source/js/public/Value.h#240-241

Hi,

Sorry for the late reply. 

Our issues flow from this wording (taken from [1]):

> When the result of any arithmetic operation other than neg, abs, or copysign is a NaN, the sign bit and the fraction field         
> (which does not include the implicit leading digit of the significand) of the NaN are computed as follows:
>
> If the fraction fields of all NaN inputs to the instruction all consist of 1 in the most significant bit and 0 in the             
> remaining bits, or if there are no NaN inputs, the result is a NaN with a nondeterministic sign bit, 1 in the most significant > bit of the fraction field, and all zeros in the remaining bits of the fraction field.
>
> Otherwise the result is a NaN with a nondeterministic sign bit, 1 in the most significant bit of the fraction field, and      > nondeterminsitic values in the remaining bits of the fraction field.

Given that the sign of NaN result is nondeterministic some of wasm tests that explicitly check for likes of "-NaN" might be incorrect but that is not MIPS specific. The issue that on pre NAN2008 MIPS hardware if arithmetic operation is given the NaN input as defined in this paragraph it will treat it as signaling NaN and result will be quiet NaN having the 0 in its most significant bit and 1 in the remaining bits. I was under the wrong impression that that jit takes this into account and optimizes fp equality checks into bitwise equality. Luckily that was plain wrong. That makes pre NAN2008 encoding hopefully an non issue for a large number of users. Whats left are the tests. I'm aware that you don't maintain a mips32/mips64 builder simulator or otherwise so it is not a direct issue for you, but it would be nice for anyone interested in MIPS to run the test w/o having to apply local changes to them in order to make them pass. Is the reasonable approach to expose a global from js shell to indicate non wasm conforming NaNs or something in order to skip parts of the wasm test that fail due to differences in NaN encoding?
Flags: needinfo?(bbouvier)
Dragan, thanks for giving me a bit more context. It sounds fine to expose a special flag for this purpose, as a jit flag for instance (look up setJitCompilerOption and see how it's used), and segregate offending tests under this flag.
Flags: needinfo?(bbouvier)
You need to log in before you can comment on or make changes to this bug.