Closed Bug 1107328 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: h4writer, Assigned: h4writer)

References

Details

Attachments

(1 file)

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.
Assignee: nobody → hv1989
Attachment #8531780 - Flags: review?(jdemooij)
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 &&.
Attachment #8531780 - Flags: review?(jdemooij) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/0b155176f4eb
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Depends on: 1115665
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.
You need to log in before you can comment on or make changes to this bug.