cpsir: fix co19 test - use process* instead of visit* (we unadvertly removed code that recursed int…

cpsir: fix co19 test - use process* instead of visit* (we unadvertly removed code that recursed into the arguments)

BUG=
R=asgerf@google.com

Committed: https://github.com/dart-lang/sdk/commit/714bf980e166e2f15ff569d25896cf8c8b785c61

[Siggi Cherem (dart-lang), 2016-01-11 21:24:48]

sigmund@google.com changed reviewers:
+ asgerf@google.com

[Siggi Cherem (dart-lang), 2016-01-11 21:24:48]

This fixes the bot failures.

[asgerf, 2016-01-11 21:35:06]

https://codereview.chromium.org/1572423002/diff/1/pkg/compiler/lib/src/cps_ir...
File pkg/compiler/lib/src/cps_ir/insert_refinements.dart (right):

https://codereview.chromium.org/1572423002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/insert_refinements.dart:130: void
processInvokeStatic(InvokeStatic node) {
Please do like in visitInvokeMethod and visitTypeTest, call the processReference
method explicitly.

processInvokeStatic is called before its operands are processed, which is the
wrong order for this pass.

Now we will be updating the refinement environment before visiting the
references so the invoke will end up seeing its own refined values. That is, we
will get code like:

x' = refine(x, int)
InvokeStatic checkInt (x')

Instead of:

InvokeStatic checkInt (x)
x' = refine(x, int)


It seems strange that this would make the test pass. Maybe it passed for the
wrong reason in the first place?

[Siggi Cherem (dart-lang), 2016-01-11 21:51:10]

PTAL

https://codereview.chromium.org/1572423002/diff/1/pkg/compiler/lib/src/cps_ir...
File pkg/compiler/lib/src/cps_ir/insert_refinements.dart (right):

https://codereview.chromium.org/1572423002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/insert_refinements.dart:130: void
processInvokeStatic(InvokeStatic node) {
On 2016/01/11 21:35:06, asgerf wrote:
> Please do like in visitInvokeMethod and visitTypeTest, call the
processReference
> method explicitly.
> 
> processInvokeStatic is called before its operands are processed, which is the
> wrong order for this pass.
> 
> Now we will be updating the refinement environment before visiting the
> references so the invoke will end up seeing its own refined values. That is,
we
> will get code like:
> 
> x' = refine(x, int)
> InvokeStatic checkInt (x')
> 
> Instead of:
> 
> InvokeStatic checkInt (x)
> x' = refine(x, int)
> 
> 
> It seems strange that this would make the test pass. Maybe it passed for the
> wrong reason in the first place?

good catch! Done.

Before I sent this I haven't yet run all the other test, just the co19 test that
failed on the bots. Turns out that the unittest for static methods did caught
this and shows exactly the issue you mention.

The co19 test was failing only because I was overriding visitInvokeStatic
without visiting the arguments entirely.

[asgerf, 2016-01-11 22:02:45]

LGTM now, though I'm still suspicious about why that test failed and now passes.

Omitting to visit references should give us worse type information, but the
types still ought to be sound. I bet  something else is interfering with the
test.

[Siggi Cherem (dart-lang), 2016-01-11 22:19:23]

Description was changed from

==========
cpsir: fix co19 test - use process* instead of visit* (we unadvertly removed
code that recursed into the arguments)

BUG=
==========

to

==========
cpsir: fix co19 test - use process* instead of visit* (we unadvertly removed
code that recursed into the arguments)

BUG=
R=asgerf@google.com

Committed:
https://github.com/dart-lang/sdk/commit/714bf980e166e2f15ff569d25896cf8c8b785c61
==========

[Siggi Cherem (dart-lang), 2016-01-11 22:19:24]

Committed patchset #2 (id:20001) manually as
714bf980e166e2f15ff569d25896cf8c8b785c61 (presubmit successful).