917200 - IonMonkey: ARM specializations of Float32 operators

Status: RESOLVED DUPLICATE of bug 913282

(Core :: JavaScript Engine, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: ARM stub patch

Now that Float32 basic support for ARM has landed, we need the ARM implementations of these supplementary operators before landing bug 913282.

I made a stub patch that allows me to cross compile locally for ARM. We just need to fill the holes now :)

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: New stub patch

This one has been already applied into the folded one in bug 913282, but this well help figuring out what are the methods to implement (principally codegen).

Comment 2

[Douglas Crosher [:dougc]]

Oops the ARM support has been posted to bug 913282.  It was only a small
matter so perhaps this one could just be closed as a duplicate.

Comment 3

[Douglas Crosher [:dougc]]

Attachment: ARM support

This was mostly a cut-and-paste of the existing float32 support.

Comment 4

[Jon Coppeard (:jonco)]

Comment on attachment 811617 [details] [diff] [review]
ARM support

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

Looks good!

::: js/src/jit/arm/CodeGenerator-arm.cpp
@@ +1652,5 @@
> +
> +    // Do the compare
> +    masm.ma_vcmpz_f32(opd);
> +    bool nocond = true;
> +    if (nocond) {

It looks like this if statement and the original visitNotD() are redundant as the condition is always true.

While you're here, can you replace the if statement by the contents of the first branch, and also for the visitNotD() one?

::: js/src/jit/arm/MacroAssembler-arm.cpp
@@ +71,5 @@
> +{
> +    // direct conversions aren't possible.
> +    VFPRegister dest = VFPRegister(dest_);
> +    as_vxfer(src, InvalidReg, dest.uintOverlay(),
> +             CoreToFloat);

nit: This doesn't need to be split over two lines.  Can you fix this and the double version too?

Comment 5

[Marty Rosenberg [:mjrosenb]]

> 
> ::: js/src/jit/arm/CodeGenerator-arm.cpp
> @@ +1652,5 @@
> > +
> > +    // Do the compare
> > +    masm.ma_vcmpz_f32(opd);
> > +    bool nocond = true;
> > +    if (nocond) {
> 
> It looks like this if statement and the original visitNotD() are redundant
> as the condition is always true.
> 
fwiw, this happened because there were two different implementations both of which seemed about equivalent in ease of implementation, and speed, so I just implemented both, with the idea that I'd benchmark both of them to see which is faster.  IIRC, there were actually three different possibilities, and one of them was behind #if 0 or something similar.

Comment 6

[Jon Coppeard (:jonco)]

(In reply to Marty Rosenberg [:mjrosenb] from comment #5)
Ah right, fair enough.  We can leave them in if you like, I just thought it would be good to tidy up.

Comment 7

[Douglas Crosher [:dougc]]

Attachment: Address reviewer feedback.

Thank you for the feedback.  I have retained the 'NotF' variations,
but added a comment to explain - just let me know if you want them
cleaned up.

My own testing has not detected any issues, but it will
get some more testing when the Odin support is added.

Carrying forward r+.

Comment 8

[Douglas Crosher [:dougc]]

Attachment: Rebased

Carrying forward r+

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Douglas, can this patch land before the series in bug 913282?
Otherwise, we might need to either fold all patches together or make this one standalone.

Comment 10

[Douglas Crosher [:dougc]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #9)
> Douglas, can this patch land before the series in bug 913282?

This patch depends on the patches in bug 913282. The patch in bug 921282 has the stubs that are filled in there.

> Otherwise, we might need to either fold all patches together or make this
> one standalone.

Yes, that might be a good option.  The patches here are applied directly after the combined patch in bug 921282 in my local source.

Comment 11

[Douglas Crosher [:dougc]]

Attachment: Rebased

Rebased to match the update to the folded patch in bug 913282.

Carrying forward r+.

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

As these bug's patches had circular dependencies with the ones in bug 913282, patches got merged and pushed on the other bug.