Clobber non-live temporaries on some paths involving a possible slow-path allocation.

Clobber non-live temporaries on some paths involving a possible slow-path allocation.

R=vegorov@google.com
BUG=

Committed: https://code.google.com/p/dart/source/detail?r=42239

[zerny-google, 2014-12-08 12:54:05]

This change caught an issue with LoadCodeUnits on ia32 and arm where I'd not
manually preserved a temp across the slow-path allocation. The clobber value
choice of 0xF7 stays in smi-range (needed to trigger a test failure), can be
loaded as an immediate on arm, and is odd.

Not all fast-paths will result in a clobber of the temps, which would require
more elaborate restructuring of the slow-path allocation calls. Any objections
to that?

PTAL

[Vyacheslav Egorov (Google), 2014-12-08 13:12:52]

LGTM but with big comments

though I thought it would be OK to leave jump around ::Allocate unclobbered and
clobber only if you actually allocate.

This way there would be no changes in the API of AllocationSlowPath.

Also AllocateOnFailure is a bit confusing. It's not exactly "failure" it's more
"out of range".

Additionally I think we should instead change SlowPath interface, to make sure
we cover that. I was thinking we do it like this:

SlowPath* some_slow_path = ...

currently we do 

j(cc, some_slow_path->entry())

instead 

some_slow_path->Jump(cc); // <- this one clobbers.

[zerny-google, 2014-12-08 16:01:23]

Thanks for the review!

On 2014/12/08 13:12:52, Vyacheslav Egorov (Google) wrote:
> LGTM but with big comments
> 
> though I thought it would be OK to leave jump around ::Allocate unclobbered
and
> clobber only if you actually allocate.
> 
> This way there would be no changes in the API of AllocationSlowPath.

Sure, that would make the change much simpler.

> Also AllocateOnFailure is a bit confusing. It's not exactly "failure" it's
more
> "out of range".

Was thinking of "negative test result = test failing", but in any case,
simplifying the CL will result in removing the function altogether :-)

> Additionally I think we should instead change SlowPath interface, to make sure
> we cover that. I was thinking we do it like this:
> 
> SlowPath* some_slow_path = ...
> 
> currently we do 
> 
> j(cc, some_slow_path->entry())
> 
> instead 
> 
> some_slow_path->Jump(cc); // <- this one clobbers.

I'm not sure I follow this last point. Since the slow-path Allocate clobbers
temps this is orthogonal no? Or is this to ensure clobbering of temps prior to
any slow-path execution?

[zerny-google, 2014-12-09 09:18:37]

Simpified the CL to only clobber temps if the slow path is taken.

[Vyacheslav Egorov (Google), 2014-12-09 11:01:40]

lgtm

[Vyacheslav Egorov (Google), 2014-12-09 17:49:46]

https://codereview.chromium.org/783103003/diff/20001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/783103003/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:1886:
locs()->live_registers()->Add(locs()->temp(0));
Actually I have been looking at this code and it does not seem like temp(0) is
live across allocation  slow path so there is no need to preserve it. 

temp(0) is used as allocation result and overwritten

[zerny-google, 2014-12-10 09:49:16]

Thanks for taking another look!

https://codereview.chromium.org/783103003/diff/20001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/783103003/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:1886:
locs()->live_registers()->Add(locs()->temp(0));
On 2014/12/09 17:49:46, Vyacheslav Egorov (Google) wrote:
> Actually I have been looking at this code and it does not seem like temp(0) is
> live across allocation  slow path so there is no need to preserve it. 
> 
> temp(0) is used as allocation result and overwritten

Your right, the updated patch set won't clobber temp. With patch-set 1, temp was
clobbered on the fast path (via AllocateOnFailure) in with the box_reg value
from EnsureMutableBox would be lost.

[zerny-google, 2014-12-10 09:51:28]

Committed patchset #3 (id:40001) manually as 42239 (presubmit successful).