532727 - Multiplication by constant 1 isn't optimized away

Status: RESOLVED DUPLICATE of bug 539876

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Maybe this isn't worth dealing with.... In the property get from arguments object code, we have

  lir->ins2(LIR_mul, idx_ins, INS_CONST(sizeof(JSTraceType)))

JSTraceType has size 1, so this is effectively a no-op.  It makes it into the generated code:

  0x00007ffff1558ec5:     imul   $0x1,%r14d,%ecx

Comment 1

[Brendan Eich [:brendan]]

We should absolutely fix this. JS_STATIC_ASSERT can handle the avoidance, or an 'if' with constant condition guarding the lir->ins2, if necessary (to be dead-code eliminated).

/be

Comment 2

[Jeff Walden [:Waldo]]

Given all the crazy mathematical-operation avoidance code in nanojit when one or both arguments are constants, it seems to me a fix should go there.  However, in the interest of avoiding the overhead here (and to simplify code a mite) we should probably fix in TM as well.

I'll take this, my recent forays in httpd.js are fast coming to a close.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Fix non-q times-one

Is there a way to write a nanojit test for an optimization like this?  I've only verified desired behavior by running lirasm --verbose on this:

[jwalden@the-great-waldo-search firefox]$ cat /tmp/mul-by-one.code 
mem = alloc 4
four = int 4
sti four mem 0
one = int 1
nonconst = ld mem 0
product1 = mul one nonconst

mem2 = alloc 4
sti four mem2 0
nonconst2 = ld mem2 0
product2 = mul nonconst2 one

sum = add product1 product2
ret sum

Comment 4

[Nicholas Nethercote [inactive]]

Comment on attachment 415968 [details] [diff] [review]
Fix non-q times-one

Fine as is, but it might be even better to move the case into the "if (c == 0)..." if-then-else just below, since this optimisation is more like those ones.

As for testing, I don't know to automatically check what you've done.  I think it's ok without a test, heaven knows none of the other similar optimisations are tested explicitly.

Also, can you run SunSpider just to make sure there's no slowdown?  Shouldn't be, but you never know with the Good Ship SS.

Comment 5

[Jeff Walden [:Waldo]]

I'd been slowly working through the pain of testing this everywhere, and it seemed to be a wash overall.  Next step would have been to push to n-c, but I wanted to test that a merge to TM would work and hadn't gotten that testing done yet.  Too late now -- seems this bug was re-reported as bug 539876, and also fixed to boot.  :-\

Well, if nothing else, the testing and building was instructive, if hopefully not something I'll need to do very often (better if done by someone who does it regularly).

Comment 6

[Nicholas Nethercote [inactive]]

Waldo, sorry for the dupe... I should have remembered reviewing your patch!