1107328 - Also test for boxed constant when folding or lowering ...

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Hannes Verschore [:h4writer]]

In Browsermark "DOM Create Source 2.1" we have some constants not getting optimized, because they are boxed. We should replace "isConstant/toConstant" with something that can handle constants and boxed constants.

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: bugxxx-constantorboxedconstant

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8531780 [details] [diff] [review]
bugxxx-constantorboxedconstant

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

Looks good, sorry for the delay!

::: js/src/jit/MIR.cpp
@@ +71,5 @@
>          fprintf(fp, "%c", tolower(name[i]));
>  }
>  
> +const Value &
> +MDefinition::constantValue() {

Nit: { on its own line, here and below. Also some trailing whitespace in these functions.

@@ +79,5 @@
> +    return toConstant()->value();
> +}
> +
> +const Value *
> +MDefinition::constantVp() {

I'd use constantValue instead of constantVp in most places. It's a bit longer but a reference is cleaner I think because it's immediately clear the value is non-null. The compiler should emit the same code for both.

@@ +90,5 @@
> +bool
> +MDefinition::constantToBoolean() {
> +    MOZ_ASSERT(isConstantValue());
> +    if (isBox())
> +        return getOperand(0)->constantVp(); 

Should call ->valueToBoolean() (another reason to use constantValue(), assuming that one doesn't compile?).

@@ +1119,5 @@
>  
>  MDefinition*
>  MStringLength::foldsTo(TempAllocator &alloc)
>  {
> +    if ((type() == MIRType_Int32) && (string()->isConstantValue())) {

Pre-existing nit: please remove the extra parentheses here on both sides of the &&.

Comment 3

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/458cfe948a05

(In reply to Jan de Mooij [:jandem] from comment #2)
> @@ +1119,5 @@
> >  
> >  MDefinition*
> >  MStringLength::foldsTo(TempAllocator &alloc)
> >  {
> > +    if ((type() == MIRType_Int32) && (string()->isConstantValue())) {
> 
> Pre-existing nit: please remove the extra parentheses here on both sides of
> the &&.

Oh I just notice I totally forgot that comment. Sorry.

Comment 4

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=4867509&repo=mozilla-inbound

Comment 5

[Hannes Verschore [:h4writer]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d56903c1c02

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b155176f4eb

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0b155176f4eb

Comment 7

[Douglas Crosher [:dougc]]

Comment on attachment 8531780 [details] [diff] [review]
bugxxx-constantorboxedconstant

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

::: js/src/jit/arm/Lowering-arm.cpp
@@ +501,5 @@
>      LAllocation ptrAlloc;
>  
>      // For the ARM it is best to keep the 'ptr' in a register if a bounds check is needed.
> +    if (ptr->isConstantValue() && !ins->needsBoundsCheck()) {
> +        int32_t ptrValue = ptr->constantVp()->toInt32();

Is it possible for the index here to be boxed? It appears to be an Odin specific path.