Cleanup how we handle side effects in HInstruction and put the right flags for HIs, modulo, truncat…

Cleanup how we handle side effects in HInstruction and put the right flags for HIs, modulo, truncate divide, and shift left.
Committed: https://code.google.com/p/dart/source/detail?r=17441

[ngeoffray, 2013-01-22 15:34:14]

This also fixes: https://code.google.com/p/dart/issues/detail?id=8015

[kasperl, 2013-01-23 07:06:21]

LGTM.

https://codereview.chromium.org/12051013/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/12051013/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2626: instruction =
new HIs(type, <HInstruction>[expression]..addAll(checks));
Not sure the type annotations on the list literals are really necessary. They do
clutter up the code somewhat.

https://codereview.chromium.org/12051013/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/ssa/nodes.dart (right):

https://codereview.chromium.org/12051013/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:1524:
{this._isStatement: false})
Is this really supposed to be a named, yet private parameter? Weird. Wouldn't it
be cleaner to have an internal constructor that takes a mandatory
this._isStatement parameter and use that internal constructor from the two other
constructors?

https://codereview.chromium.org/12051013/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:2099: :
super(<HInstruction>[receiver]) {
It's almost like having a constructor on HInstruction that clears side effects
and enables GVN would be useful. Then you could use:

    : super.noSideEffects(...)

but you should probably only do it if it's used in enough places.

[ngeoffray, 2013-01-23 09:23:01]

Thanks Kasper, let me know if you disagree with my replies.

https://codereview.chromium.org/12051013/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/12051013/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2626: instruction =
new HIs(type, <HInstruction>[expression]..addAll(checks));
On 2013/01/23 07:06:21, kasperl wrote:
> Not sure the type annotations on the list literals are really necessary. They
do
> clutter up the code somewhat.

I actually prefer keeping them, making sure checked mode will catch any invalid
store into the list.

https://codereview.chromium.org/12051013/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/ssa/nodes.dart (right):

https://codereview.chromium.org/12051013/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:1524:
{this._isStatement: false})
On 2013/01/23 07:06:21, kasperl wrote:
> Is this really supposed to be a named, yet private parameter? Weird. Wouldn't
it
> be cleaner to have an internal constructor that takes a mandatory
> this._isStatement parameter and use that internal constructor from the two
other
> constructors?

For statically resolved methods, I prefer putting boolean parameters into named
parameters, instead of having these true/false all around. For this class, the
_isStatement field does not have to be more private than the foreignType field,
so I removed the privacy. Is that OK with you?

https://codereview.chromium.org/12051013/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:2099: :
super(<HInstruction>[receiver]) {
On 2013/01/23 07:06:21, kasperl wrote:
> It's almost like having a constructor on HInstruction that clears side effects
> and enables GVN would be useful. Then you could use:
> 
>     : super.noSideEffects(...)
> 
> but you should probably only do it if it's used in enough places.

There are two HInstructions that would benefit from this constructor. My
preference would be to keep the assignment of flags local to make it consistent.

[kasperl, 2013-01-23 09:31:26]

https://codereview.chromium.org/12051013/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/ssa/nodes.dart (right):

https://codereview.chromium.org/12051013/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:1524:
{this._isStatement: false})
On 2013/01/23 09:23:01, ngeoffray wrote:
> On 2013/01/23 07:06:21, kasperl wrote:
> > Is this really supposed to be a named, yet private parameter? Weird.
Wouldn't
> it
> > be cleaner to have an internal constructor that takes a mandatory
> > this._isStatement parameter and use that internal constructor from the two
> other
> > constructors?
> 
> For statically resolved methods, I prefer putting boolean parameters into
named
> parameters, instead of having these true/false all around. For this class, the
> _isStatement field does not have to be more private than the foreignType
field,
> so I removed the privacy. Is that OK with you?

Yes!

https://codereview.chromium.org/12051013/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:2099: :
super(<HInstruction>[receiver]) {
On 2013/01/23 09:23:01, ngeoffray wrote:
> On 2013/01/23 07:06:21, kasperl wrote:
> > It's almost like having a constructor on HInstruction that clears side
effects
> > and enables GVN would be useful. Then you could use:
> > 
> >     : super.noSideEffects(...)
> > 
> > but you should probably only do it if it's used in enough places.
> 
> There are two HInstructions that would benefit from this constructor. My
> preference would be to keep the assignment of flags local to make it
consistent.

Sounds good!