512652 - TM: refactor Assembler::evict()

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: patch

This patch does some refactoring of Assembler::evict().  The process was:

- evict() contained a call to registerAlloc(rmask(r)).  So I cloned
  registerAlloc() and then specialised the clone for the case where the
  argument is a single register.  
  
- I did likewise for findVictim() within the clone, and inlined the
  result.
  
- I then inlined the cloned registerAlloc() within evict(), the only place that
  called it, and did some more local optimisations and rearrangements.
  
- I then split evict() into maybeEvict() and evict().

The end result is that less code is run because it's specialised for
evicting a single register.  Also, even though there's slightly more source
code I think it's easier to understand because there's less indirection and
layers of function calls involved.

Comment 1

[Edwin Smith]

Comment on attachment 396684 [details] [diff] [review]
patch

looks ok to me but adding Rick for a second set of eyes.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: revised patch

This just tweaks the ordering in evict() -- the previous patch caused the debug output to be slightly wrong, because when printing the state that precedes the eviction instruction the evicted register was still shown as live.

Comment 3

[Rick Reitmaier]

Comment on attachment 397140 [details] [diff] [review]
revised patch

Interesting that the semantics of maybeEvict() lead to the 'used' bit being set in the case of register being free?!?

We should probably add 'NanoAssert(_allocator.getActive(r));' in this case, as I'm guessing that we'd have a bug if this was not true.

Or possibly the register is used as a temp and it will be free'd shortly.

Comment 4

[Nicholas Nethercote [inactive]]

(In reply to comment #3)
> (From update of attachment 397140 [details] [diff] [review])
> Interesting that the semantics of maybeEvict() lead to the 'used' bit being set
> in the case of register being free?!?

Don't blame me, I preserved the meaning of what was there :)  Having said that, whether the 'used' field is correct is unclear but it is scheduled for removal anyway, see bug 512398.

> We should probably add 'NanoAssert(_allocator.getActive(r));' in this case, as
> I'm guessing that we'd have a bug if this was not true.

By "this case" you mean the "then" branch of the if-then-else in maybeEvict()?  If so, definitely not -- if a register is free it cannot be active.  I think 'used' is misleading, probably wrong, and best ignored!

Comment 5

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/f38f30f2888b

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/f38f30f2888b

Comment 7

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/cc7a84f61dc0