1013420 - Factor redundant floating-point algorithms in CodeGenerators

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine: JIT, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

When implementing the float variants for some algorithms (e.g. round, floor), I've directly copied and pasted code from the Double variant. As these methods are very similar, modulo the names of the used macro-asm operations (e.g. addsd / addss), maybe we can factor them out without too much overkill. This bug is an attempt at doing that refactoring.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1 - Cleanup

This patch rewrites round / floor generation code to make it easier to read, by deleting a few indent levels.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Part 2 - Factor out Floor code

So, that's the way I am seeing it. This patch introduces more lines than it deletes, but it makes code cleaner IMO. Plus, the more uses of these templatized functions we'll have, the more deletions there'll be in comparison in the future patches. At the end, we might even paste the body of the called function, rather than calling it (e.g. replacing fcompare<double> body by compareDouble's body).

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Part 3 - Factor out Round

Another example of such refactoring.

Comment 4

[Dan Gohman [:sunfish]]

Comment on attachment 8425625 [details] [diff] [review]
Part 2 - Factor out Floor code

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

::: js/src/jit/shared/Assembler-x86-shared.cpp
@@ +44,5 @@
> +}
> +template<>
> +void AssemblerX86Shared::fround<float>(FloatRegister src, FloatRegister dest, JSC::X86Assembler::RoundingMode mode) {
> +    roundss(src, dest, mode);
> +}

It'd be nice if all these trivial wrapper functions could be defined inline. If you use the inline keyword, you can safely move these definitions into the header file.

::: js/src/jit/shared/Assembler-x86-shared.h
@@ +1521,5 @@
>          JS_ASSERT(HasSSE2());
>          masm.xorps_rr(src.code(), dest.code());
>      }
> +    template<class T> void
> +    fxorp(FloatRegister src, FloatRegister dest);

I know I was the one who suggested prefix f as the naming convention, but seeing it here and elsewhere, it's less pretty than I initially thought. What if we used the convention of replacing the suffix [sd] with the suffix X? Then we'd have xorpX, roundsX, etc? That might provide a more obvious mapping down to the instruction names.

Crazy idea: What if we got rid of xorpd and xorps as functions, and just had fxorp/xorpX be the primary interface everyone uses directly? I'm not necessarily sold on this idea myself, I just wanted to bring it up as an option to help reduce the number of wrapper functions.

Comment 5

[Dan Gohman [:sunfish]]

Comment on attachment 8425626 [details] [diff] [review]
Part 3 - Factor out Round

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

::: js/src/jit/IonMacroAssembler.cpp
@@ +39,5 @@
> +{
> +    loadConstantDouble(val, dest);
> +}
> +template<>
> +void MacroAssembler::floadConstant<float>(double val, FloatRegister dest)

Should val be float here?

@@ +41,5 @@
> +}
> +template<>
> +void MacroAssembler::floadConstant<float>(double val, FloatRegister dest)
> +{
> +    loadConstantFloat32(float(val), dest);

... and then you can drop this cast

::: js/src/jit/IonMacroAssembler.h
@@ +774,5 @@
>      // This function clobbers the input register.
>      void clampDoubleToUint8(FloatRegister input, Register output);
>  
> +    template<class T>
> +    void floadConstant(double val, FloatRegister dest);

... and make val be T here

::: js/src/jit/shared/CodeGenerator-x86-shared.cpp
@@ +1799,4 @@
>      Label negative, end, bailout;
>  
>      // Load 0.5 in the temp register.
> +    masm.floadConstant<T>(0.5, temp);

... and then this use T(0.5) here?

I can't decide if I like this better, or if I like the current code because you could conveniently add an assert that casting to float doesn't alter the value.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8425625 [details] [diff] [review]
Part 2 - Factor out Floor code

Thanks for the first comments! I'll go back to that once bug 1010747 has landed, to spare myself some rebasing sessions.

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8425617 [details] [diff] [review]
1 - Cleanup

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

Looks good.

::: js/src/jit/shared/CodeGenerator-x86-shared.cpp
@@ +1907,5 @@
>  
> +    // No SSE4.1
> +    masm.addsd(input, temp);
> +    // Round toward -Infinity without the benefit of ROUNDSD.
> +    // If input + 0.5 >= 0, input is a negative number >= -0.5 and the result is -0.

Nit: combine the two comments, and add \n before the other line:

// No SSE4.1, round toward -Infinity without the benefit of ROUNDSD.
masm.addsd(input, temp);

// If input + 0.5 >= 0, input is a negative number >= -0.5 and the result is -0.

@@ +1994,4 @@
>  
> +    // No SSE4.1
> +    masm.addss(input, temp);
> +    // Round toward -Infinity without the benefit of ROUNDSS.

Same here.

Comment 8

[Lars T Hansen [:lth]]

Benjamin, can you decide if we still want this (need to replace templates with overloading in the .h files, not much work) and if so, give it a priority and assign it to yourself, or if not, close it?

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Pretty sure I've answered yesterday, probably forgot to submit :(

Anyways, wontfix:
- not high priority
- the wrappers are not making this look better
- not a net win in number of lines of code
- make the masm expose a templated API, which we don't want
- these algorithms are kinda blackboxed: once they work, we usually don't need to touch them again
- any discrepancy between float32 and float64 would impose to undo refactoring

Who proposed that idea anyway? :)