Validate that all instructions dominate their inputs. And fix a bug where that did not happen.

Validate that all instructions dominate their inputs. And fix a bug where that did not happen.
Committed: https://code.google.com/p/dart/source/detail?r=8089

[floitsch, 2012-05-29 14:50:45]

Please add a test case. Helps to figure out what went wrong.

[ngeoffray, 2012-05-29 14:52:40]

I didn't add a new test, because our existing tests with try/catch/finally are
failing the validator. I could list the ones that fail the validator if you're
interested.

[floitsch, 2012-05-29 18:31:37]

L mostly GTM.
There is still one part (getActivationValue) I haven't figured out yet, but I
hope it will be easy once you have added a comment.

https://chromiumcodereview.appspot.com/10454049/diff/4008/lib/compiler/implem...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10454049/diff/4008/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:440: if (element.isParameter())
return directLocals[element];
Please add comment what is happening here.

https://chromiumcodereview.appspot.com/10454049/diff/4008/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:3136: localsHandler = new
LocalsHandler.from(savedLocals);
Add comment:
The catch block must not reuse the existing LocalsHandler. None of the variables
that have been defined in the body-block will be used, but for loops we will add
(unnecessary) phis that will reference the body variables. This will make it
look as if the variables were used in a non-dominated block.

[ngeoffray, 2012-05-30 08:31:45]

Thanks Florian

https://chromiumcodereview.appspot.com/10454049/diff/4008/lib/compiler/implem...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10454049/diff/4008/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:440: if (element.isParameter())
return directLocals[element];
On 2012/05/29 18:31:38, floitsch wrote:
> Please add comment what is happening here.

Done.

https://chromiumcodereview.appspot.com/10454049/diff/4008/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:3136: localsHandler = new
LocalsHandler.from(savedLocals);
On 2012/05/29 18:31:38, floitsch wrote:
> Add comment:
> The catch block must not reuse the existing LocalsHandler. None of the
variables
> that have been defined in the body-block will be used, but for loops we will
add
> (unnecessary) phis that will reference the body variables. This will make it
> look as if the variables were used in a non-dominated block.

Done.

[Lasse Reichstein Nielsen, 2012-05-30 09:35:14]

LGTM

https://chromiumcodereview.appspot.com/10454049/diff/9001/lib/compiler/implem...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10454049/diff/9001/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:3126: // variables. This will make
it look as if the variables were used
"will make" -> "makes" (prefer active voice where possible)

https://chromiumcodereview.appspot.com/10454049/diff/9001/lib/compiler/implem...
File lib/compiler/implementation/ssa/closure.dart (right):

https://chromiumcodereview.appspot.com/10454049/diff/9001/lib/compiler/implem...
lib/compiler/implementation/ssa/closure.dart:439: bool oldInTryCatchOrFinally =
inTryCatchOrFinally;
Odd name. Could it be just "inTryStatement" since the catch and finally blocks
are part of the "try" statement?

https://chromiumcodereview.appspot.com/10454049/diff/9001/lib/compiler/implem...
File lib/compiler/implementation/ssa/nodes.dart (right):

https://chromiumcodereview.appspot.com/10454049/diff/9001/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:666: while (current !== null) {
A block must have a non-null first instruction (right?).
So you could make this a do-while instead and save one comparison :)

https://chromiumcodereview.appspot.com/10454049/diff/9001/lib/compiler/implem...
File lib/compiler/implementation/ssa/tracer.dart (right):

https://chromiumcodereview.appspot.com/10454049/diff/9001/lib/compiler/implem...
lib/compiler/implementation/ssa/tracer.dart:26: String method;
Is this used?

https://chromiumcodereview.appspot.com/10454049/diff/9001/lib/compiler/implem...
File lib/compiler/implementation/ssa/validate.dart (right):

https://chromiumcodereview.appspot.com/10454049/diff/9001/lib/compiler/implem...
lib/compiler/implementation/ssa/validate.dart:87: // corresponding predecessor
block.
We usually use "dominating" about blocks, not instructions.
Maybe say that the parameters/inputs' blocks dominate the corresponding
predecessor blocks (with a comment that blocks dominate themselves).

https://chromiumcodereview.appspot.com/10454049/diff/9001/lib/compiler/implem...
lib/compiler/implementation/ssa/validate.dart:100: for (HInstruction input in
instruction.inputs) {
This is an un-pretty combination of functional for-each and iterator based
for-each. How about making blocks/instruction-lists Iterable<HInstruction>
instead?
Then you can do
 for (HPhi phi in block.phis)
and
 for (HInstruction instruction in block)

[ngeoffray, 2012-05-30 10:19:06]

Thanks Lasse!

New CL for addressing the comments:
https://chromiumcodereview.appspot.com/10446080

https://chromiumcodereview.appspot.com/10454049/diff/9001/lib/compiler/implem...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10454049/diff/9001/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:3126: // variables. This will make
it look as if the variables were used
On 2012/05/30 09:35:14, Lasse Reichstein Nielsen wrote:
> "will make" -> "makes" (prefer active voice where possible)

Done.

https://chromiumcodereview.appspot.com/10454049/diff/9001/lib/compiler/implem...
File lib/compiler/implementation/ssa/closure.dart (right):

https://chromiumcodereview.appspot.com/10454049/diff/9001/lib/compiler/implem...
lib/compiler/implementation/ssa/closure.dart:439: bool oldInTryCatchOrFinally =
inTryCatchOrFinally;
On 2012/05/30 09:35:14, Lasse Reichstein Nielsen wrote:
> Odd name. Could it be just "inTryStatement" since the catch and finally blocks
> are part of the "try" statement?

Done.

https://chromiumcodereview.appspot.com/10454049/diff/9001/lib/compiler/implem...
File lib/compiler/implementation/ssa/nodes.dart (right):

https://chromiumcodereview.appspot.com/10454049/diff/9001/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:666: while (current !== null) {
On 2012/05/30 09:35:14, Lasse Reichstein Nielsen wrote:
> A block must have a non-null first instruction (right?).
> So you could make this a do-while instead and save one comparison :)

It might be safer to keep it as-is in case one use forEachInstruction while
building (and the block does not have any instruction).

https://chromiumcodereview.appspot.com/10454049/diff/9001/lib/compiler/implem...
File lib/compiler/implementation/ssa/tracer.dart (right):

https://chromiumcodereview.appspot.com/10454049/diff/9001/lib/compiler/implem...
lib/compiler/implementation/ssa/tracer.dart:26: String method;
On 2012/05/30 09:35:14, Lasse Reichstein Nielsen wrote:
> Is this used?

Leftover debugging. Removed.

https://chromiumcodereview.appspot.com/10454049/diff/9001/lib/compiler/implem...
File lib/compiler/implementation/ssa/validate.dart (right):

https://chromiumcodereview.appspot.com/10454049/diff/9001/lib/compiler/implem...
lib/compiler/implementation/ssa/validate.dart:87: // corresponding predecessor
block.
On 2012/05/30 09:35:14, Lasse Reichstein Nielsen wrote:
> We usually use "dominating" about blocks, not instructions.
> Maybe say that the parameters/inputs' blocks dominate the corresponding
> predecessor blocks (with a comment that blocks dominate themselves).

Done.

https://chromiumcodereview.appspot.com/10454049/diff/9001/lib/compiler/implem...
lib/compiler/implementation/ssa/validate.dart:100: for (HInstruction input in
instruction.inputs) {
On 2012/05/30 09:35:14, Lasse Reichstein Nielsen wrote:
> This is an un-pretty combination of functional for-each and iterator based
> for-each. How about making blocks/instruction-lists Iterable<HInstruction>
> instead?
> Then you can do
>  for (HPhi phi in block.phis)
> and
>  for (HInstruction instruction in block)

As discussed, it doesn't look un-pretty to me, as I usually use forEach on
non-array data structures, and forin (or for (int i....)) on arrays.

[floitsch, 2012-05-30 11:52:38]

https://chromiumcodereview.appspot.com/10454049/diff/9001/lib/compiler/implem...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10454049/diff/9001/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:440: // If the element is a
parameter, we already have a
Can you also add a comment, why we have this 'if'?
Unless I'm wrong this is an optimization to avoid redundant assignments.
Correct?

[ngeoffray, 2012-05-30 12:37:26]

https://chromiumcodereview.appspot.com/10454049/diff/9001/lib/compiler/implem...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10454049/diff/9001/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:440: // If the element is a
parameter, we already have a
On 2012/05/30 11:52:38, floitsch wrote:
> Can you also add a comment, why we have this 'if'?
> Unless I'm wrong this is an optimization to avoid redundant assignments.
> Correct?

There is a comment:
If the element is a parameter, we already have a HParameterValue for it.

It's just to avoid adding unnecessary new HParameterValue. So I'm not sure what
you think is missing from the comment?

[floitsch, 2012-05-30 12:59:40]

https://chromiumcodereview.appspot.com/10454049/diff/9001/lib/compiler/implem...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10454049/diff/9001/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:440: // If the element is a
parameter, we already have a
On 2012/05/30 12:37:26, ngeoffray wrote:
> On 2012/05/30 11:52:38, floitsch wrote:
> > Can you also add a comment, why we have this 'if'?
> > Unless I'm wrong this is an optimization to avoid redundant assignments.
> > Correct?
> 
> There is a comment:
> If the element is a parameter, we already have a HParameterValue for it.
> 
> It's just to avoid adding unnecessary new HParameterValue. So I'm not sure
what
> you think is missing from the comment?

If I'm not wrong this is just to avoid wrapping parameters. Removing this line
would not change anything.
So what I would like to see (as comment): is this necessary, or just an
optimization.

[ngeoffray, 2012-05-31 08:18:33]

https://chromiumcodereview.appspot.com/10454049/diff/9001/lib/compiler/implem...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10454049/diff/9001/lib/compiler/implem...
lib/compiler/implementation/ssa/builder.dart:440: // If the element is a
parameter, we already have a
On 2012/05/30 12:59:41, floitsch wrote:
> On 2012/05/30 12:37:26, ngeoffray wrote:
> > On 2012/05/30 11:52:38, floitsch wrote:
> > > Can you also add a comment, why we have this 'if'?
> > > Unless I'm wrong this is an optimization to avoid redundant assignments.
> > > Correct?
> > 
> > There is a comment:
> > If the element is a parameter, we already have a HParameterValue for it.
> > 
> > It's just to avoid adding unnecessary new HParameterValue. So I'm not sure
> what
> > you think is missing from the comment?
> 
> If I'm not wrong this is just to avoid wrapping parameters. Removing this line
> would not change anything.
> So what I would like to see (as comment): is this necessary, or just an
> optimization.

It is necessary because we need to share the same name between the
HParameterValue (real parameter) and the HFieldGet and HFieldSet instructions.
I'll add that as a comment.