Fix variable allocator in the presence of HCheck instructions.

Fix variable allocator in the presence of HCheck instructions.

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

[ngeoffray, 2013-04-16 14:58:02]

https://codereview.chromium.org/13877009/diff/4001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/variable_allocator.dart
(left):

https://codereview.chromium.org/13877009/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/variable_allocator.dart:239: if
(environment.contains(instruction)) return;
The contains check here and the code line 151 were the root cause of the bug:
line 151 was adding an instruction to the live environment, and when we started
analyzing that instruction we would not run the code below. The code below
setups ranges for the inputs of an instruction.

[karlklose, 2013-04-17 07:48:22]

I'll do a full review when I get into the office, just one comment now: I think
the test should be in dart2js_extra, since it is a regression test for an
internal detail of dart2js code generation.

It would be even better if we could write a unit test, because global type
inference may invalidate the assumptions documented in the comments.

[Mads Ager (google), 2013-04-17 08:05:48]

Thank you Nicolas! LGTM but I have swapped most of this out and Karl should do
the actual approval. :-)

[ngeoffray, 2013-04-17 08:19:18]

On 2013/04/17 07:48:22, karlklose wrote:
> I'll do a full review when I get into the office, just one comment now: I
think
> the test should be in dart2js_extra, since it is a regression test for an
> internal detail of dart2js code generation.

My preference has been to have dart2js_extra contain tests that test dart2js's
behavior when it can't follow the spec. If you look at language tests, it
contains tests with names issue*.dart. Those tests could be for the VM or
dart2js. Also, the VM does not have such a vm_extra directory. So the general
approach has been to have all tests that should pass on the VM and dartj2s in
language tests.

> 
> It would be even better if we could write a unit test, because global type
> inference may invalidate the assumptions documented in the comments.

This does not have to be true, if we have the @inscrutable suggested by
Stephen/Peter. But I agree a unit test could be better here. We just don't have
the mock infrastructure for it today, but it's been on my radar for a while.

[karlklose, 2013-04-17 08:43:47]

LGTM.

[ngeoffray, 2013-04-17 09:05:59]

Committed patchset #1 manually as r21607 (presubmit successful).