Closed Bug 532727 Opened 14 years ago Closed 14 years ago

Multiplication by constant 1 isn't optimized away


(Core :: JavaScript Engine, defect)

Not set





(Reporter: bzbarsky, Assigned: Waldo)



(1 file)

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
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).

Assignee: general → dvander
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.
Assignee: dvander → jwalden+bmo
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
Attachment #415968 - Flags: review?(nnethercote)
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.
Attachment #415968 - Flags: review?(nnethercote) → review+
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).
Closed: 14 years ago
Resolution: --- → DUPLICATE
Waldo, sorry for the dupe... I should have remembered reviewing your patch!
You need to log in before you can comment on or make changes to this bug.