1060635 - optimize MMinMax with constant operand

Status: NEW

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

Description

[Luke Wagner [:luke]]

For Math.min(K, 1.0), where K is known to be a double, we currently generate:

   movsd 0x15f4(%rip),%xmm1
   ucomisd %xmm1,%xmm0
   jne a
   jp b
   orpd %xmm1,%xmm0
   jmpq c
b: ucomisd %xmm0,%xmm0
   jp c
a: minsd  %xmm1,%xmm0
c:

I assume most of this has to do with nans and negative zeros and things.  Perhaps we could do better when one operand is a constant?  gcc generates a single 'minsd'.

Also, for MinMax of ints, where one is a constant, gcc generates a cmov.

This saturation pattern showed up in the hot spot of a real image manipulation workload.

Comment 1

[sajjadt]

Attachment: Patch for generating int32 cmoves

I've attached a patch for replacing "integer cmp/branch/move"s with "cmp/cmove"s. We ended up using one more register when one operand is constant.

Comment 2

[sajjadt]

Attachment: A patch for optimizing MaxMin with a const value

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8630234 [details] [diff] [review]
A patch for optimizing MaxMin with a const value

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

Thanks for this contribution! You might want to take a look at the style guidelines [0], to avoid a lot of comments about the coding style not being followed ;)

Also, don't forget to set the review field to somebody, so that your patches get some attention. You can do so by changing the "review" flag to "?", and then enter the nickname of a JS peer prefixed with a colon (for instance, I'm bbouvier on IRC => my reviewer's name is :bbouvier). (Note that this trick works for people who set their nickname as such in bugzilla). For this patch, I've set feedback+, meaning that this is a good start, but it needs a few enhancements before getting the holy r+ (necessary for checking in).

When uploading your next revision of the patch, you can also set the content of the file as "patch" to enhance the Splinter view for reviewing. I'll do it manually for this one.

The code looks good at a first glance. Checking for isConstant is nice for a start, but I think range analysis results could be used to generalize this code. For your updated patch, please choose :nbp or :h4writer as the reviewers, as they have more background about that. Keep up the good work!

[0] https://wiki.mozilla.org/JavaScript:SpiderMonkey:Coding_Style

::: js/src/jit/Lowering.cpp
@@ +1270,5 @@
>        case MIRType_Float32:
> +        // If second operand is constant, it should be kept in read-write register, so
> +        // X86's vminsd and vmaxsd instructions produce expected results in case of
> +        // NaNs (i.e they return read-only register)
> +        if(second->isConstant())

another way to do it would be to use mozilla::Swap(first, second) if second->isConstant()

@@ +1277,1 @@
>          lir = new(alloc()) LMinMaxF(useRegisterAtStart(first), useRegister(second));

nit: next line must be indented too

@@ +1281,5 @@
> +        // X86's vminsd and vmaxsd instructions produce expected results in case of
> +        // NaNs (i.e they return read-only register)
> +        if (second->isConstant())
> +            lir = new(alloc()) LMinMaxD(useRegisterAtStart(second), useRegister(first));
> +        else

same remarks apply here

::: js/src/jit/x86-shared/CodeGenerator-x86-shared.cpp
@@ +573,4 @@
>  
>      Label done, nan, minMaxInst;
>  
> +    bool infiniteOrNaNConstant = false ;

nit: here and below, no spaces before ending semicolon

@@ +574,5 @@
>      Label done, nan, minMaxInst;
>  
> +    bool infiniteOrNaNConstant = false ;
> +    bool noConstant = true ;
> +    

nit: here and below a lot of trailing spaces

@@ +580,5 @@
> +    MDefinition* secondOperand = ins->mir()->getOperand(1);
> +    
> +    if (firstOperand->isConstant()) {
> +        Value val = firstOperand->toConstant()->value() ;
> +        infiniteOrNaNConstant = !mozilla::IsFinite( val.toDouble() ) ;

nit: here and below, no spaces before/after parenthesis in function calls

@@ +593,5 @@
> +
> +    // Following tests are not necessary if one operands is a finite constant
> +    // since variable that could be NaN,Inf is already placed in read-only operand
> +    if (noConstant || infiniteOrNaNConstant) {
> +        

nit: trailing whitespace + you need to change the indentation level in this |if| body (here and below)

@@ +649,5 @@
>  
> +    bool infiniteOrNaNConstant = false ;
> +    bool noConstant = true ;
> +    
> +    

nit: 2 blank lines, please remove one

@@ +656,5 @@
> +    
> +    if (firstOperand->isConstant()) {
> +        Value val = firstOperand->toConstant()->value() ;
> +        infiniteOrNaNConstant = !mozilla::IsFinite( val.toDouble() ) ;
> +        noConstant = false ;

I'd rather name it "hasConstantOperand" and invert its values, so as not to have to deal with double negation (noSomething = false).

@@ +667,5 @@
> +    }
> +    
> +    // Following tests are not necessary if one operands is a finite constant
> +    // since variable that could be NaN,Inf is already placed in read-only operand
> +    if (noConstant || infiniteOrNaNConstant) {

Could we use only one boolean here?

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Actually, I see you already set the file as a patch, my bad. However, it was missing some context: the commit message should have the following format: 

`Bug ${bug number} - ${quick description of the changes}`

Also, my pretty standard mercurial settings for diff are the following:
[diff]
git = 1
unified = 8
showfunc = 1

I suggest you also add this to your .hgrc if you're using mercurial, or the equivalent configuration in your favorite CVS ;)

Comment 5

[sajjadt]

Thanks Benjamin for the review and advices. highly appreciated :). Will make another patch.

Comment 6

[sajjadt]

Attachment: bug_1060635.patch

This patch replace cmp/branch/set with cmp/conditional move when generating MinMax(I) for all the platforms. Additinoally, it optimizes MinMax(D/F) for X86/64 when one operand is a constant.

Comment 7

[Sean Stangl [:sstangl]]

Comment on attachment 8640816 [details] [diff] [review]
bug_1060635.patch

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

Looks good overall, just some minor cleanup and flipping some values around. Can give the next patch to Ben, since he did the initial reviews.

::: js/src/jit/CodeGenerator.cpp
@@ +5188,5 @@
>      Assembler::Condition cond = ins->mir()->isMax()
> +                                ? Assembler::LessThan
> +                                : Assembler::GreaterThan;
> +
> +    masm.condMove32(cond, ToRegister(ins->second()), output);

To me, this reads backwards. In the isMax() case, this reads:

"If ins->second() < output, output = ins->second()".

It looks like maybe a comparison got flipped somewhere inside condMove32()?

::: js/src/jit/Lowering.cpp
@@ +1270,5 @@
>        case MIRType_Float32:
> +        // If second operand is constant, it should be kept in read-write register, so
> +        // X86's vminsd and vmaxsd instructions produce expected results in case of
> +        // NaNs (i.e they return read-only register)
> +        if(second->isConstant())

nit: "if ("

@@ +1279,5 @@
>        case MIRType_Double:
> +        // If second operand is constant, it should be kept in read-write register, so
> +        // X86's vminsd and vmaxsd instructions produce expected results in case of
> +        // NaNs (i.e they return read-only register)
> +        if(second->isConstant())

nit: "if ("

@@ +1281,5 @@
> +        // X86's vminsd and vmaxsd instructions produce expected results in case of
> +        // NaNs (i.e they return read-only register)
> +        if(second->isConstant())
> +             mozilla::Swap(second, first);
> +             

nit: horizontal whitespace

::: js/src/jit/x86-shared/CodeGenerator-x86-shared.cpp
@@ +583,2 @@
>      }
> +    if (secondOperand->isConstant()) {

} else if {

Why do we have to check the second operand to be a constant, anyway? If there are two constants, we should have constant-folded, and the pass in Lowering moved any constants to the first operand. If anything, we should MOZ_ASSERT(!secondOperand->isConstant()).

@@ +586,5 @@
> +        finiteConstant = mozilla::IsFinite(val.toDouble());
> +    }
> +
> +    // Following steps are not necessary if one operands is a finite constant
> +    // since the variable that could be NaN,Inf is already placed in read-only operand

This probably could be clearer to specify the behavior of VMAXSD/VMINSD below, and why it's safe if the NaN/Inf is in the second operand. Looks valid to me, though.

@@ +588,5 @@
> +
> +    // Following steps are not necessary if one operands is a finite constant
> +    // since the variable that could be NaN,Inf is already placed in read-only operand
> +    if (!finiteConstant) {
> +

nit: excess vertical whitespace

@@ +650,5 @@
>      }
> +    if (secondOperand->isConstant()) {
> +        Value val = secondOperand->toConstant()->value();
> +        finiteConstant = mozilla::IsFinite(val.toDouble());
> +    }

Same comments on this function, including all the nits and whitespace stuff.

::: js/src/jit/x86-shared/MacroAssembler-x86-shared.h
@@ +108,5 @@
>      void branchNegativeZero(FloatRegister reg, Register scratch, Label* label, bool  maybeNonZero = true);
>      void branchNegativeZeroFloat32(FloatRegister reg, Register scratch, Label* label);
>  
> +    void condMove32(Condition cond, Register src, Register dest) {
> +        cmp32(dest, src);

This looks strange, shouldn't it be (src, dest)? Normally for -32 and -Ptr functions we try to preserve argument order so that a comparison (GreaterThan, A, B) means (A GreaterThan B).

::: js/src/jit/arm/MacroAssembler-arm.cpp
@@ +2050,5 @@
> +// Conditional move
> +void
> +MacroAssemblerARMCompat::condMove32(Condition cond, Register src, Register dest)
> +{
> +    ma_cmp(dest, src);

This should absolutely be |ma_cmp(src, dest)| to mimic branch32() ordering.

@@ +2051,5 @@
> +void
> +MacroAssemblerARMCompat::condMove32(Condition cond, Register src, Register dest)
> +{
> +    ma_cmp(dest, src);
> +    ma_mov(src, dest, 1, cond);

No hardcoded constants: "SetCC" instead of "1".

::: js/src/jit/arm64/MacroAssembler-arm64.h
@@ +396,5 @@
>          }
>      }
> +
> +    void condMove32(Condition cond, Register src, Register dest) {
> +        Cmp(ARMRegister(dest, 32), ARMRegister(src, 32));

cmp32(src, dest)

::: js/src/jit/mips/MacroAssembler-mips.h
@@ +390,5 @@
>      }
>  
> +    // Conditional move
> +    void condMove32(Condition cond, Register src, Register dst) {
> +        ma_cmp_set(ScratchRegister, src, dst, cond);

Not sure, but based on all the other architectures this probably needs to be flipped also.

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:sdetar, could you have a look please?
For more information, please visit auto_nag documentation.