Closed Bug 1211150 Opened 7 years ago Closed 7 years ago

ARM: Adding 'explicit' keyword on the FloatRegister constructors.

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: victorcarlquist, Unassigned)

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Please, see bug 984018, comment 95.
Attachment #8669339 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8669339 [details] [diff] [review]
Part 0 - Added 'explicit' keyword on FloatRegister in ARM64.

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

Thanks for working on this issue :)

::: js/src/jit/arm64/Architecture-arm64.h
@@ +318,5 @@
>          float s;
>          double d;
>      };
>  
> +    constexpr explicit FloatRegister(uint32_t code, FloatRegisters::Kind k)

no need to add explicit on constructors which have more than one argument.

::: js/src/jit/arm64/Assembler-arm64.h
@@ +33,5 @@
>  
>  static constexpr Register ScratchReg2 = { Registers::ip1 };
>  static constexpr ARMRegister ScratchReg2_64 = { ScratchReg2, 64 };
>  
> +static constexpr FloatRegister ScratchDoubleReg { FloatRegisters::d31 };

This does not match the subject of this patch and seems to be an unrelated modification.
Also, if we do so, I would prefer if we could do it on all architectures at once.
Attachment #8669339 - Flags: review?(nicolas.b.pierron)
(In reply to Nicolas B. Pierron [:nbp] from comment #3)

> ::: js/src/jit/arm64/Assembler-arm64.h
> @@ +33,5 @@
> >  
> >  static constexpr Register ScratchReg2 = { Registers::ip1 };
> >  static constexpr ARMRegister ScratchReg2_64 = { ScratchReg2, 64 };
> >  
> > +static constexpr FloatRegister ScratchDoubleReg { FloatRegisters::d31 };
> 
> This does not match the subject of this patch and seems to be an unrelated
> modification.

We need to modify this to compile correctly, but it will break the code style, because the Register class doesn't have the explicit keyword too.

So, I am wonder if this patch is really necessary.
Flags: needinfo?(nicolas.b.pierron)
(In reply to Victor Carlquist from comment #4)
> (In reply to Nicolas B. Pierron [:nbp] from comment #3)
> 
> > ::: js/src/jit/arm64/Assembler-arm64.h
> > @@ +33,5 @@
> > >  
> > >  static constexpr Register ScratchReg2 = { Registers::ip1 };
> > >  static constexpr ARMRegister ScratchReg2_64 = { ScratchReg2, 64 };
> > >  
> > > +static constexpr FloatRegister ScratchDoubleReg { FloatRegisters::d31 };
> > 
> > This does not match the subject of this patch and seems to be an unrelated
> > modification.
> 
> We need to modify this to compile correctly, but it will break the code
> style, because the Register class doesn't have the explicit keyword too.
> 
> So, I am wonder if this patch is really necessary.

I am not sure to understand why we need this notation while we never required it on any other architecture, can you explain me what difference this makes?  Maybe we should do the same on other architecture as well.
Flags: needinfo?(nicolas.b.pierron)
(In reply to Nicolas B. Pierron [:nbp] from comment #5)
> (In reply to Victor Carlquist from comment #4)
> I am not sure to understand why we need this notation while we never
> required it on any other architecture, can you explain me what difference
> this makes?  Maybe we should do the same on other architecture as well.

Sure, 
when we added the explicit keyword on FloatRegister contructor, the notation with the '=' symbol (indirect initialization) will fail, because the compiler does not consider this constructor with the indirect initialization, just with direct initialization [1]. 

So, we can use the following notations to initialize the object:

without the '='

> FloatRegister ScratchDoubleReg { FloatRegisters::d31 };

or

> FloatRegister ScratchDoubleReg(FloatRegisters::d31);

Maybe the second notation is clearer than the first one.

The Register class doesn't have the explicit keyword, so it works with the '='.

[1] http://en.cppreference.com/w/cpp/language/explicit
Flags: needinfo?(nicolas.b.pierron)
Hum … I guess we did not see this issue on other architecture because we define the FloatRegister::Kind as well as the register.  I guess we should probably do the same here.
Flags: needinfo?(nicolas.b.pierron)
(In reply to Nicolas B. Pierron [:nbp] from comment #7)
> Hum … I guess we did not see this issue on other architecture because we
> define the FloatRegister::Kind as well as the register.  I guess we should
> probably do the same here.

Yes! You are right, thank you!
Attached patch PatchSplinter Review
Hi,
We need to modify just the ARM64 because the ARM32 already has the 'explicit' keyword and the mips, x86 and x64 don't have a constructor with one argument.

Thanks again!
Attachment #8669339 - Attachment is obsolete: true
Attachment #8672818 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8672818 [details] [diff] [review]
Patch

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

This patch makes the FloatRegister explicit, and in addition explicit the register kind used for each register which highlight existing issues as d0 is only the encoding of the register and not the code (type + encoding).

::: js/src/jit/arm64/Assembler-arm64.h
@@ +64,5 @@
>  static constexpr ARMRegister ZeroRegister64 = { Registers::sp, 64 };
>  static constexpr ARMRegister ZeroRegister32 = { Registers::sp, 32 };
>  
> +static constexpr FloatRegister ReturnFloatReg = { FloatRegisters::d0, FloatRegisters::Single };
> +static constexpr FloatRegister ScratchFloatReg = { FloatRegisters::d31, FloatRegisters::Single };

This sounds like a bug in the current implementation.  We should probably remove these 2 as they are supposed to be aliases of ScratchDoubleReg and ReturnDoubleReg.

Can you open a follow-up bug and CC :nbp and :jolesen on it.

::: js/src/jit/arm64/SharedICRegisters-arm64.h
@@ +50,5 @@
>  // since we keep the return address for calls there.
>  
>  // FloatReg0 must be equal to ReturnFloatReg.
> +static constexpr FloatRegister FloatReg0 = { FloatRegisters::v0, FloatRegisters::Single };
> +static constexpr FloatRegister FloatReg1 = { FloatRegisters::v1, FloatRegisters::Single };

Same thing for these one, these are supposed to be vector registers, which we do not yet have a float register kind for in Architecture-arm64.h.
Attachment #8672818 - Flags: review?(nicolas.b.pierron) → review+
Keywords: checkin-needed
Hi, can we get a try run here ?
Flags: needinfo?(victorcarlquist)
Keywords: checkin-needed
(In reply to Carsten Book [:Tomcat] from comment #11)
> Hi, can we get a try run here ?

Ops! I forgot it,
do I still need to post a try run here?
Flags: needinfo?(victorcarlquist)
(In reply to Victor Carlquist from comment #13)
> (In reply to Carsten Book [:Tomcat] from comment #11)
> > Hi, can we get a try run here ?
> 
> Ops! I forgot it,
> do I still need to post a try run here?

I don't think this would be needed as the patch landed a few hours ago (comment 12). Otherwise, as a rule of thumb, you should consider doing it before setting the checkin-needed keyword.
https://hg.mozilla.org/mozilla-central/rev/71eac1ec50ff
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.