Fix regression on side effects of tdiv.

Fix regression on side effects of tdiv.

R=sra@google.com

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

[sra1, 2013-12-11 09:45:23]

I'm pretty confused here.

https://codereview.chromium.org/111553003/diff/1/tests/compiler/dart2js/mock_...
File tests/compiler/dart2js/mock_compiler.dart (right):

https://codereview.chromium.org/111553003/diff/1/tests/compiler/dart2js/mock_...
tests/compiler/dart2js/mock_compiler.dart:155: // right thing.
I'm not sure 'do the right thing' is what we really mean here.
My understanding is that the side effect below models a shortcomming of the
effect analysis on the real library.  This is quite obscure and will become
inaccurate if effect analysis is changed in almost any way.
The comment should say all this, because the code below is very fragile and
unintuitive in its connection to the real code.

Or maybe I misunderstand.

https://codereview.chromium.org/111553003/diff/1/tests/compiler/dart2js/side_...
File tests/compiler/dart2js/side_effect_tdiv_regression_test.dart (right):

https://codereview.chromium.org/111553003/diff/1/tests/compiler/dart2js/side_...
tests/compiler/dart2js/side_effect_tdiv_regression_test.dart:28:
Expect.isTrue(generated.contains('return c + c;'));
How does adding the side effect in the mock library cause this to be correct?

[ngeoffray, 2013-12-16 13:31:41]

PTAL.

https://codereview.chromium.org/111553003/diff/1/tests/compiler/dart2js/mock_...
File tests/compiler/dart2js/mock_compiler.dart (right):

https://codereview.chromium.org/111553003/diff/1/tests/compiler/dart2js/mock_...
tests/compiler/dart2js/mock_compiler.dart:155: // right thing.
On 2013/12/11 09:45:24, sra1 wrote:
> I'm not sure 'do the right thing' is what we really mean here.
> My understanding is that the side effect below models a shortcomming of the
> effect analysis on the real library.  This is quite obscure and will become
> inaccurate if effect analysis is changed in almost any way.
> The comment should say all this, because the code below is very fragile and
> unintuitive in its connection to the real code.
> 
> Or maybe I misunderstand.

No, that's correct. I improved the comment.

https://codereview.chromium.org/111553003/diff/1/tests/compiler/dart2js/side_...
File tests/compiler/dart2js/side_effect_tdiv_regression_test.dart (right):

https://codereview.chromium.org/111553003/diff/1/tests/compiler/dart2js/side_...
tests/compiler/dart2js/side_effect_tdiv_regression_test.dart:28:
Expect.isTrue(generated.contains('return c + c;'));
On 2013/12/11 09:45:24, sra1 wrote:
> How does adding the side effect in the mock library cause this to be correct?

In order to get a.field GVN'ed, we need to see that ~/ does not have side
effects. If the mock library version does not have side effects, then the code
to fix the regression would not be necessary.

[sra1, 2013-12-18 00:07:32]

lgtm

[ngeoffray, 2013-12-18 07:35:19]

Committed patchset #2 manually as r31212 (presubmit successful).