Introduce AbstractValue and AbstractValueDomain

This is part of a move towards being able to replace TypeMask. First step is replacing TypeMask usage with interface that do not imply any particular way of abstracting runtime values. This CL is the first part of this first step.

This CL:
- renames the existing AbstractValue in cps_ir/type_propagation.dart to AbstractConstant
- introduces AbstractValue as an empty superinterface of TypeMask, and
- lift the interface of TypeMaskSystem to the superinterface AbstractValueDomain.

In follow-ups:
- change simple uses from TypeMask(System) to AbstractValue(Domain)
- update names and comment in AbstractValueDomain to more precisely reflect the abstraction
- update the complex uses (mainly type_propagation.dart) to use AbstractValue(Domain)

R=asgerf@google.com

Committed: https://github.com/dart-lang/sdk/commit/1328c98cc2b9c8a4704375753626fe34c569325e

[Johnni Winther, 2015-10-22 13:27:29]

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

[Johnni Winther, 2015-10-22 13:27:29]

New classes in 
   types/runtime_value_abstraction.dart 
and adopted in 
   cps_ir/type_mask_system.dart
   cps_ir/cps_ir_nodes.dart
   types/type_mask.dart

ScalarReplacer has been updated to use the new classes instead of TypeMask to
showcase the intent.

[asgerf, 2015-10-22 15:26:18]

I'm OK with the design, but I'm concerned that CPS will become stuck mid-way in
an undergoing transition from one interface to another.

Assuming the ReceiverConstrant thing gets resolved, I would prefer to update all
the CPS code at once instead of a half-way transition.

https://codereview.chromium.org/1415883005/diff/1/pkg/compiler/lib/src/types/...
File pkg/compiler/lib/src/types/runtime_value_abstraction.dart (right):

https://codereview.chromium.org/1415883005/diff/1/pkg/compiler/lib/src/types/...
pkg/compiler/lib/src/types/runtime_value_abstraction.dart:8: abstract class
RuntimeValue {}
Shouldn't this implement ReceiverConstraint? Or there should be a
"toReceiverConstrant" in RuntimeValueAbstraction.

Right now we get a downcast to ReceiverConstraint in codegen when we register
dynamic invocations.

[Siggi Cherem (dart-lang), 2015-10-22 22:06:40]

Good point Asger. I'm also curious about whether we'll change our mind about how
to do this once we move our global inference to run on top of CPS rather than
using its own IR. At that point maybe we'll prefer to always keep this
information on the side or to update the same slot that we use here during type
propagation, I honestly don't know.

I'm OK with making the change first and then updating what we do later, but also
might be OK to wait until we get some additional clarity.

https://codereview.chromium.org/1415883005/diff/1/pkg/compiler/lib/src/types/...
File pkg/compiler/lib/src/types/runtime_value_abstraction.dart (right):

https://codereview.chromium.org/1415883005/diff/1/pkg/compiler/lib/src/types/...
pkg/compiler/lib/src/types/runtime_value_abstraction.dart:7: /// A value in an
abstraction of runtime values.
naming nits: I'd think of the entity here to be a runtime value abstraction,
since it abstracts a runtime value.

some ideas:
RuntimeValue => AbstractRuntimeValue? or AbstractValue?
RuntimeValueAbstraction => AbstractValueDomain? AbstractDomain?

https://codereview.chromium.org/1415883005/diff/1/pkg/compiler/lib/src/types/...
pkg/compiler/lib/src/types/runtime_value_abstraction.dart:8: abstract class
RuntimeValue {}
On 2015/10/22 15:26:18, asgerf wrote:
> Shouldn't this implement ReceiverConstraint? Or there should be a
> "toReceiverConstrant" in RuntimeValueAbstraction.
> 
> Right now we get a downcast to ReceiverConstraint in codegen when we register
> dynamic invocations.

Or maybe ReceiverConstraint implements RuntimeValue?

[Johnni Winther, 2015-11-30 10:32:32]

Description was changed from

==========
Introduce RuntimeValue and RuntimeValueAbstraction

The plan is decouple TypeMask from the system by using RuntimeValue instead of
TypeMask and RuntimeValueAbstraction instead of TypeMaskSystem (the one in
cps_ir).

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

to

==========
This CL:
- renames the existing AbstractValue in cps_ir/type_propagation.dart to
AbstractConstant
- introduces AbstractValue as an empty superinterface of TypeMask, and
- lift the interface of TypeMaskSystem to the superinterface
AbstractValueDomain.

In follow-ups:
- change simple uses from TypeMask(System) to AbstractValue(Domain)
- update names and comment in AbstractValueDomain to more precisely reflect the
abstraction
- update the complex uses (mainly type_propagation.dart) to use
AbstractValue(Domain)
==========

[Johnni Winther, 2015-11-30 10:32:44]

PTAL

[Johnni Winther, 2015-12-02 08:50:54]

ping

[asgerf, 2015-12-02 11:38:31]

Sorry about the delay. LGTM.

https://codereview.chromium.org/1415883005/diff/20001/pkg/compiler/lib/src/ty...
File pkg/compiler/lib/src/types/abstract_value_domain.dart (right):

https://codereview.chromium.org/1415883005/diff/20001/pkg/compiler/lib/src/ty...
pkg/compiler/lib/src/types/abstract_value_domain.dart:24: /// A system that
implements an abstraction over runtime values.
"...and provides access to interprocedural analysis results."

Inferred types are usually separate from the domain itself. One could implement
a global type inference parameterized by an abstract value domain, but not if
the domain itself needs access to the results of this analysis.

It's something we might want to change, but until then, let's clarify that this
is in fact more than just an abstract domain.

[Johnni Winther, 2015-12-02 12:52:17]

Description was changed from

==========
This CL:
- renames the existing AbstractValue in cps_ir/type_propagation.dart to
AbstractConstant
- introduces AbstractValue as an empty superinterface of TypeMask, and
- lift the interface of TypeMaskSystem to the superinterface
AbstractValueDomain.

In follow-ups:
- change simple uses from TypeMask(System) to AbstractValue(Domain)
- update names and comment in AbstractValueDomain to more precisely reflect the
abstraction
- update the complex uses (mainly type_propagation.dart) to use
AbstractValue(Domain)
==========

to

==========
This CL:
- renames the existing AbstractValue in cps_ir/type_propagation.dart to
AbstractConstant
- introduces AbstractValue as an empty superinterface of TypeMask, and
- lift the interface of TypeMaskSystem to the superinterface
AbstractValueDomain.

In follow-ups:
- change simple uses from TypeMask(System) to AbstractValue(Domain)
- update names and comment in AbstractValueDomain to more precisely reflect the
abstraction
- update the complex uses (mainly type_propagation.dart) to use
AbstractValue(Domain)

R=asgerf@google.com

Committed:
https://github.com/dart-lang/sdk/commit/1328c98cc2b9c8a4704375753626fe34c569325e
==========

[Johnni Winther, 2015-12-02 12:52:19]

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

[Johnni Winther, 2015-12-02 13:15:07]

https://codereview.chromium.org/1415883005/diff/20001/pkg/compiler/lib/src/ty...
File pkg/compiler/lib/src/types/abstract_value_domain.dart (right):

https://codereview.chromium.org/1415883005/diff/20001/pkg/compiler/lib/src/ty...
pkg/compiler/lib/src/types/abstract_value_domain.dart:24: /// A system that
implements an abstraction over runtime values.
On 2015/12/02 11:38:31, asgerf wrote:
> "...and provides access to interprocedural analysis results."
> 
> Inferred types are usually separate from the domain itself. One could
implement
> a global type inference parameterized by an abstract value domain, but not if
> the domain itself needs access to the results of this analysis.
> 
> It's something we might want to change, but until then, let's clarify that
this
> is in fact more than just an abstract domain.

Done. Added a TODO for splitting the interface.

[sra1, 2015-12-02 17:42:23]

sra@google.com changed reviewers:
+ sra@google.com

[sra1, 2015-12-02 17:42:24]

I have a 'rude' question.

Why we need this extra layer of abstract classes called AbstractSomething? 
Perhaps you could expand the CL comment to include motivation?

https://codereview.chromium.org/1415883005/diff/40001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/type_propagation.dart (right):

https://codereview.chromium.org/1415883005/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:46: final
AbstractConstantValue anything;
I find 'AbstractConstantValue' confusing.  The abstract value here is not a
constant, as witnessed by the .isConstant property.

https://codereview.chromium.org/1415883005/diff/40001/pkg/compiler/lib/src/ty...
File pkg/compiler/lib/src/types/abstract_value_domain.dart (right):

https://codereview.chromium.org/1415883005/diff/40001/pkg/compiler/lib/src/ty...
pkg/compiler/lib/src/types/abstract_value_domain.dart:28: abstract class
AbstractValueDomain {
With these concrete types there will only be one instance of this interface.
Do we really need to have an interface that we must edit every time we change
type_propagation?

I could understand what you are doing if this was
abstract class AbstractDomain<AbstractValue> ...

Still, that takes the very generic names (AbstractValue) and uses them
specifically for types.

[asgerf, 2015-12-02 18:05:07]

https://codereview.chromium.org/1415883005/diff/40001/pkg/compiler/lib/src/ty...
File pkg/compiler/lib/src/types/abstract_value_domain.dart (right):

https://codereview.chromium.org/1415883005/diff/40001/pkg/compiler/lib/src/ty...
pkg/compiler/lib/src/types/abstract_value_domain.dart:28: abstract class
AbstractValueDomain {
On 2015/12/02 17:42:24, sra1 wrote:
> With these concrete types there will only be one instance of this interface.
> Do we really need to have an interface that we must edit every time we change
> type_propagation?
> 
> I could understand what you are doing if this was
> abstract class AbstractDomain<AbstractValue> ...
> 
> Still, that takes the very generic names (AbstractValue) and uses them
> specifically for types.

FWIW, I think of this as AbstractDomain<T>, but in practice it's horrible to use
it in that way because everything that uses the abstract domain would then also
need to be parameterized by <T> or just use `var` when talking about abstract
values.

Using an empty interface is just slightly better than using `var` everywhere and
not as invasive as using <T> everywhere.

[Johnni Winther, 2015-12-03 09:05:16]

On 2015/12/02 17:42:24, sra1 wrote:
> I have a 'rude' question.
> 
> Why we need this extra layer of abstract classes called AbstractSomething? 
> Perhaps you could expand the CL comment to include motivation?
> 

This is part of a move towards being able to replace TypeMask. First step is
replacing TypeMask usage with interface that do not imply any particular way of
abstracting runtime values. This CL is the first part of this first step.

[Johnni Winther, 2015-12-03 09:05:32]

Description was changed from

==========
This CL:
- renames the existing AbstractValue in cps_ir/type_propagation.dart to
AbstractConstant
- introduces AbstractValue as an empty superinterface of TypeMask, and
- lift the interface of TypeMaskSystem to the superinterface
AbstractValueDomain.

In follow-ups:
- change simple uses from TypeMask(System) to AbstractValue(Domain)
- update names and comment in AbstractValueDomain to more precisely reflect the
abstraction
- update the complex uses (mainly type_propagation.dart) to use
AbstractValue(Domain)

R=asgerf@google.com

Committed:
https://github.com/dart-lang/sdk/commit/1328c98cc2b9c8a4704375753626fe34c569325e
==========

to

==========
This is part of a move towards being able to replace TypeMask. First step is
replacing TypeMask usage with interface that do not imply any particular way of
abstracting runtime values. This CL is the first part of this first step.

This CL:
- renames the existing AbstractValue in cps_ir/type_propagation.dart to
AbstractConstant
- introduces AbstractValue as an empty superinterface of TypeMask, and
- lift the interface of TypeMaskSystem to the superinterface
AbstractValueDomain.

In follow-ups:
- change simple uses from TypeMask(System) to AbstractValue(Domain)
- update names and comment in AbstractValueDomain to more precisely reflect the
abstraction
- update the complex uses (mainly type_propagation.dart) to use
AbstractValue(Domain)

R=asgerf@google.com

Committed:
https://github.com/dart-lang/sdk/commit/1328c98cc2b9c8a4704375753626fe34c569325e
==========