DBC: Various instructions.

DBC: Various instructions.

- LoadIndexed
- EqualityCompare
- RelationalOp

R=vegorov@google.com

Committed: https://github.com/dart-lang/sdk/commit/726fd84ff4f768e2d27a6e3fe999000092317d07

[zra, 2016-06-30 06:04:09]

zra@google.com changed reviewers:
+ vegorov@google.com

[zra, 2016-06-30 06:04:10]

Adding these instructions seems to expose a bug in exceptions thrown from
inlined functions. When running:

out/DebugSIMDBC64/dart  --optimization_counter_threshold=10
--no-background-compilation --ignore-unrecognized-flags
--package-root=out/DebugSIMDBC64/packages/
runtime/tests/vm/dart/inline_stack_frame_test.dart

I get the stack frames only for func2 and func1, not for func1 through func5 as
expected.

Any idea where to look for this one? Thanks!

[Vyacheslav Egorov (Google), 2016-06-30 15:55:14]

On 2016/06/30 06:04:10, zra wrote:
> Adding these instructions seems to expose a bug in exceptions thrown from
> inlined functions. When running:
> 
> out/DebugSIMDBC64/dart  --optimization_counter_threshold=10
> --no-background-compilation --ignore-unrecognized-flags
> --package-root=out/DebugSIMDBC64/packages/
> runtime/tests/vm/dart/inline_stack_frame_test.dart
> 
> I get the stack frames only for func2 and func1, not for func1 through func5
as
> expected.
> 
> Any idea where to look for this one? Thanks!

This usually means that inlined function iterator does not work for some reason.
Try starting at InlinedFunctionsIterator::InlinedFunctionsIterator. 

My guess would be that call at which we through does not have a lazy deopt
associated with it and code_.GetDeoptInfoAtPc returns null.

[Vyacheslav Egorov (Google), 2016-06-30 15:55:26]

Also some preliminary comments

https://codereview.chromium.org/2113563002/diff/1/runtime/vm/intermediate_lan...
File runtime/vm/intermediate_language_dbc.cc (right):

https://codereview.chromium.org/2113563002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_dbc.cc:582: #if defined(PRODUCT)
Consider adding a helper method on the Instruction that encapsulates this.

Instruction::Unsupported() {
...
}

https://codereview.chromium.org/2113563002/diff/1/runtime/vm/simulator_dbc.cc
File runtime/vm/simulator_dbc.cc (right):

https://codereview.chromium.org/2113563002/diff/1/runtime/vm/simulator_dbc.cc...
runtime/vm/simulator_dbc.cc:2321: RawArray* array =
static_cast<RawArray*>(FP[rB]);
Consider using RAW_CAST instead of assertions below.

(static_cast + assertions was an outdated way to write this code introduced
before RAW_CAST was introduced)

https://codereview.chromium.org/2113563002/diff/1/runtime/vm/simulator_dbc.cc...
runtime/vm/simulator_dbc.cc:2326: FP[rA] = *(array->ptr()->data() +
Smi::Value(index));
I suggest array->ptr()->data[Smi::Value(index)] here.

[zra, 2016-06-30 17:32:32]

https://codereview.chromium.org/2113563002/diff/1/runtime/vm/intermediate_lan...
File runtime/vm/intermediate_language_dbc.cc (right):

https://codereview.chromium.org/2113563002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_dbc.cc:582: #if defined(PRODUCT)
On 2016/06/30 15:55:26, Vyacheslav Egorov (Google) wrote:
> Consider adding a helper method on the Instruction that encapsulates this.
> 
> Instruction::Unsupported() {
> ...
> }

Doing this here: https://codereview.chromium.org/2111803003/

https://codereview.chromium.org/2113563002/diff/1/runtime/vm/simulator_dbc.cc
File runtime/vm/simulator_dbc.cc (right):

https://codereview.chromium.org/2113563002/diff/1/runtime/vm/simulator_dbc.cc...
runtime/vm/simulator_dbc.cc:2321: RawArray* array =
static_cast<RawArray*>(FP[rB]);
On 2016/06/30 15:55:26, Vyacheslav Egorov (Google) wrote:
> Consider using RAW_CAST instead of assertions below.
> 
> (static_cast + assertions was an outdated way to write this code introduced
> before RAW_CAST was introduced)

Done here, and some others that I saw.

https://codereview.chromium.org/2113563002/diff/1/runtime/vm/simulator_dbc.cc...
runtime/vm/simulator_dbc.cc:2326: FP[rA] = *(array->ptr()->data() +
Smi::Value(index));
On 2016/06/30 15:55:26, Vyacheslav Egorov (Google) wrote:
> I suggest array->ptr()->data[Smi::Value(index)] here.

Done.

[Vyacheslav Egorov (Google), 2016-07-01 16:15:36]

https://codereview.chromium.org/2113563002/diff/20001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_dbc.cc (right):

https://codereview.chromium.org/2113563002/diff/20001/runtime/vm/intermediate...
runtime/vm/intermediate_language_dbc.cc:1215: if (labels.fall_through !=
labels.false_label) {
I don't get how this works. If label.fall_through is equal to labels.false_label
that means NEXT_IS_FALSE.

https://codereview.chromium.org/2113563002/diff/20001/runtime/vm/intermediate...
runtime/vm/intermediate_language_dbc.cc:1245: BranchLabels labels = { &is_true,
&is_false, &is_false };
This does not match the code below. This configuration of labels means that
falltrough is false case.

https://codereview.chromium.org/2113563002/diff/20001/runtime/vm/intermediate...
runtime/vm/intermediate_language_dbc.cc:1282: BranchLabels labels = { &is_true,
&is_false, &is_false };
The same issue again. You are falling through to a true case while labels say
you are falling through to the false case.

https://codereview.chromium.org/2113563002/diff/20001/runtime/vm/intermediate...
runtime/vm/intermediate_language_dbc.cc:1302: __ IfLe(length, index);
This is not correct. This does not check for negative index, code in other
back-ends is using *unsigned*.

[zra, 2016-07-01 17:43:45]

https://codereview.chromium.org/2113563002/diff/20001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_dbc.cc (right):

https://codereview.chromium.org/2113563002/diff/20001/runtime/vm/intermediate...
runtime/vm/intermediate_language_dbc.cc:1215: if (labels.fall_through !=
labels.false_label) {
On 2016/07/01 16:15:35, Vyacheslav Egorov (Google) wrote:
> I don't get how this works. If label.fall_through is equal to
labels.false_label
> that means NEXT_IS_FALSE.

Added some comments here and to EmitBranchOnCondition.

https://codereview.chromium.org/2113563002/diff/20001/runtime/vm/intermediate...
runtime/vm/intermediate_language_dbc.cc:1245: BranchLabels labels = { &is_true,
&is_false, &is_false };
On 2016/07/01 16:15:35, Vyacheslav Egorov (Google) wrote:
> This does not match the code below. This configuration of labels means that
> falltrough is false case.

The labels don't make sense here because we aren't jumping. They're just picked
so that EmitComparisonCode will return NEXT_IS_TRUE. I'll add a comment.

https://codereview.chromium.org/2113563002/diff/20001/runtime/vm/intermediate...
runtime/vm/intermediate_language_dbc.cc:1282: BranchLabels labels = { &is_true,
&is_false, &is_false };
On 2016/07/01 16:15:35, Vyacheslav Egorov (Google) wrote:
> The same issue again. You are falling through to a true case while labels say
> you are falling through to the false case.

Acknowledged.

https://codereview.chromium.org/2113563002/diff/20001/runtime/vm/intermediate...
runtime/vm/intermediate_language_dbc.cc:1302: __ IfLe(length, index);
On 2016/07/01 16:15:35, Vyacheslav Egorov (Google) wrote:
> This is not correct. This does not check for negative index, code in other
> back-ends is using *unsigned*.

Do you have a preference whether we should have unsigned comparison
instructions, or add a flag to IfLe saying signed or unsigned?

[zra, 2016-07-01 20:55:39]

https://codereview.chromium.org/2113563002/diff/20001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_dbc.cc (right):

https://codereview.chromium.org/2113563002/diff/20001/runtime/vm/intermediate...
runtime/vm/intermediate_language_dbc.cc:1302: __ IfLe(length, index);
On 2016/07/01 17:43:45, zra wrote:
> On 2016/07/01 16:15:35, Vyacheslav Egorov (Google) wrote:
> > This is not correct. This does not check for negative index, code in other
> > back-ends is using *unsigned*.
> 
> Do you have a preference whether we should have unsigned comparison
> instructions, or add a flag to IfLe saying signed or unsigned?

Added unsigned comparison instructions. We can revisit if we're running short on
instructions.

[Vyacheslav Egorov (Google), 2016-07-07 16:42:06]

LGTM

Thanks for the comments. I got confused by what NEXT_ means.

https://codereview.chromium.org/2113563002/diff/60001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_dbc.cc (right):

https://codereview.chromium.org/2113563002/diff/60001/runtime/vm/intermediate...
runtime/vm/intermediate_language_dbc.cc:1257: BranchLabels labels = { &is_true,
&is_false, &is_false };
I see. NEXT_IS_FALSE actually refers to instruction after instruction that
follows condition. 

CondOp ...
Op0
Op1        <- NEXT_IS_... refers to this instruction not to the Op0 which is
immediately *next* after the condition itself.

In a sense this makes sense if we consider CondOp + one instruction immediately
after it as a single meta-instruction.

[zra, 2016-07-11 16:02:21]

Description was changed from

==========
DBC: Various instructions.

- LoadIndexed
- EqualityCompare
- RelationalOp
==========

to

==========
DBC: Various instructions.

- LoadIndexed
- EqualityCompare
- RelationalOp

R=vegorov@google.com

Committed:
https://github.com/dart-lang/sdk/commit/726fd84ff4f768e2d27a6e3fe999000092317d07
==========

[zra, 2016-07-11 16:02:22]

Committed patchset #4 (id:60001) manually as
726fd84ff4f768e2d27a6e3fe999000092317d07 (presubmit successful).