Support Int32 representation for selected binary operations.

Support Int32 representation for selected binary operations.

BUG=
R=fschneider@google.com

Committed: https://code.google.com/p/dart/source/detail?r=39595

[Vyacheslav Egorov (Google), 2014-08-26 22:48:37]

vegorov@google.com changed reviewers:
+ fschneider@google.com, johnmccutchan@google.com, srdjan@google.com

[Vyacheslav Egorov (Google), 2014-08-26 22:48:37]

I think this is ready for the review.

Only ARM and x86 support.

You can notice that BinaryInt32Op is almost identical copy of BinarySmiOp, I am
going to start cleaning this stuff next. However this is the shortest way to
actually land this stuff - start by duplication.

Next step would be to disentangle and unify BinaryInt32Op, BinaryUint32Op,
BinarySmiOp instruction patterns and cleanup the notion of truncation through
out IR. But it will take a while before I figure out fine details of the
unification.

[Florian Schneider, 2014-08-27 09:36:52]

LGTM.

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/compiler.cc#n...
runtime/vm/compiler.cc:415: // comming from the loop pre-header into smi-phis.
s/comming/coming/

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/flow_graph_op...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:601: if (phi != NULL) {
Can you simplify the computation of deopt_target and check for int32-phis here?

e.g. 

if (phi != NULL && to != kUnboxedInt32) {
  insert_before = ...
  deopt_target = NULL;
} else {
  deopt_target = insert_before = ...
}

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:4677: return true;
For left and right shift we can save untagging of the shift count, if it is not
a compile-time constant.

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:4732: // values is using kUnboxedInt32
representaion instead of kTagged.
s/representaion/representation/

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/flow_graph_ra...
File runtime/vm/flow_graph_range_analysis.h (right):

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/flow_graph_ra...
runtime/vm/flow_graph_range_analysis.h:425:
!max().UpperBound().Overflowed(size);
I prefer line break before the &&:

    return !min().LowerBound().Overflowed(size)
        && !max().UpperBound().Overflowed(size);

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/flow_graph_ra...
File runtime/vm/flow_graph_range_analysis_test.cc (right):

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/flow_graph_ra...
runtime/vm/flow_graph_range_analysis_test.cc:622: #if 0
Please add back test cases.

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.h:6997: #if 0
Dead code?

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.h:8090: // virtual void
InferRange(RangeAnalysis* analysis, Range* range);
Remove commented code.

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.h:8110: // virtual CompileType ComputeType()
const;
Remove commented code.

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_arm.cc:3352: if (value == 0) {
TODO: Shift by 0 constant should be folded away. I assume the same issue exists
for mint/smi operations.

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_arm.cc:3474: __ b(deopt);
It seems wrong to always deoptimize in this case. The same issue exists in the
existing smi operation. Let's use the opportunity to document things that need
clean up.

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_arm.cc:3580: __ b(deopt);
Add TODO here as well.

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:3188: asm("int $3");
Remove debugging left-over?

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:3290: UNREACHABLE();
UNIMPLEMENTED?

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:3409: UNREACHABLE();
UNIMPLEMENTED?

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:3413: UNREACHABLE();
UNIMPLEMENTED?

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:3417: UNREACHABLE();
UNIMPLEMENTED?

https://codereview.chromium.org/504143003/diff/80001/tests/standalone/unboxed...
File tests/standalone/unboxed_int_converter_test.dart (right):

https://codereview.chromium.org/504143003/diff/80001/tests/standalone/unboxed...
tests/standalone/unboxed_int_converter_test.dart:12: for (var i = a; i <= b;
i++) {
This test does not seem to cover all int32 operations. e.g. do we have any test
coverage with int32 phis?

[Vyacheslav Egorov (Google), 2014-08-27 11:45:37]

Thanks for the review. Addressing comments and landing.

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/compiler.cc#n...
runtime/vm/compiler.cc:415: // comming from the loop pre-header into smi-phis.
On 2014/08/27 09:36:50, Florian Schneider wrote:
> s/comming/coming/

Done.

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/flow_graph_op...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:601: if (phi != NULL) {
On 2014/08/27 09:36:51, Florian Schneider wrote:
> Can you simplify the computation of deopt_target and check for int32-phis
here?
> 
> e.g. 
> 
> if (phi != NULL && to != kUnboxedInt32) {
>   insert_before = ...
>   deopt_target = NULL;
> } else {
>   deopt_target = insert_before = ...
> }

Done.

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:4677: return true;
On 2014/08/27 09:36:51, Florian Schneider wrote:
> For left and right shift we can save untagging of the shift count, if it is
not
> a compile-time constant.

Done.

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/flow_graph_op...
runtime/vm/flow_graph_optimizer.cc:4732: // values is using kUnboxedInt32
representaion instead of kTagged.
On 2014/08/27 09:36:51, Florian Schneider wrote:
> s/representaion/representation/

Done.

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/flow_graph_ra...
File runtime/vm/flow_graph_range_analysis.h (right):

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/flow_graph_ra...
runtime/vm/flow_graph_range_analysis.h:425:
!max().UpperBound().Overflowed(size);
On 2014/08/27 09:36:51, Florian Schneider wrote:
> I prefer line break before the &&:
> 
>     return !min().LowerBound().Overflowed(size)
>         && !max().UpperBound().Overflowed(size);

I will just aligned ! signs.

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/flow_graph_ra...
File runtime/vm/flow_graph_range_analysis_test.cc (right):

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/flow_graph_ra...
runtime/vm/flow_graph_range_analysis_test.cc:622: #if 0
On 2014/08/27 09:36:51, Florian Schneider wrote:
> Please add back test cases.

Done.

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.h:8090: // virtual void
InferRange(RangeAnalysis* analysis, Range* range);
On 2014/08/27 09:36:51, Florian Schneider wrote:
> Remove commented code.

Done.

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.h:8110: // virtual CompileType ComputeType()
const;
On 2014/08/27 09:36:51, Florian Schneider wrote:
> Remove commented code.

Done.

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_arm.cc:3352: if (value == 0) {
On 2014/08/27 09:36:51, Florian Schneider wrote:
> TODO: Shift by 0 constant should be folded away. I assume the same issue
exists
> for mint/smi operations.

Yes, I have this as part of my BinarySmiOpInstr refactoring: I am going to
remove all value comparisons and do them in the Canonicalize pass.

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_arm.cc:3474: __ b(deopt);
On 2014/08/27 09:36:51, Florian Schneider wrote:
> It seems wrong to always deoptimize in this case. The same issue exists in the
> existing smi operation. Let's use the opportunity to document things that need
> clean up.

Yes, I was also surprised we do it this way and as the result penalize all the
code.

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:3188: asm("int $3");
On 2014/08/27 09:36:51, Florian Schneider wrote:
> Remove debugging left-over?

Done.

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:3290: UNREACHABLE();
On 2014/08/27 09:36:51, Florian Schneider wrote:
> UNIMPLEMENTED?

UNREACHABLE() is more meaningful because supported operations are filtered
outside by IsSupported.

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:3409: UNREACHABLE();
On 2014/08/27 09:36:51, Florian Schneider wrote:
> UNIMPLEMENTED?

Ditto.

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:3413: UNREACHABLE();
On 2014/08/27 09:36:51, Florian Schneider wrote:
> UNIMPLEMENTED?

Ditto

https://codereview.chromium.org/504143003/diff/80001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:3417: UNREACHABLE();
On 2014/08/27 09:36:51, Florian Schneider wrote:
> UNIMPLEMENTED?

Ditto.

https://codereview.chromium.org/504143003/diff/80001/tests/standalone/unboxed...
File tests/standalone/unboxed_int_converter_test.dart (right):

https://codereview.chromium.org/504143003/diff/80001/tests/standalone/unboxed...
tests/standalone/unboxed_int_converter_test.dart:12: for (var i = a; i <= b;
i++) {
On 2014/08/27 09:36:51, Florian Schneider wrote:
> This test does not seem to cover all int32 operations. e.g. do we have any
test
> coverage with int32 phis?

both `sum` and `j` are int32 phis here. 

I will add all supported operations into this test.

[Florian Schneider, 2014-08-27 14:38:14]

lgtm

[Vyacheslav Egorov (Google), 2014-08-27 14:49:41]

Committed patchset #6 manually as 39595 (presubmit successful).

[srdjan, 2014-08-27 16:17:13]

DBC and thanks for quick implementation.

https://codereview.chromium.org/504143003/diff/100001/runtime/vm/deopt_instru...
File runtime/vm/deopt_instructions.cc (right):

https://codereview.chromium.org/504143003/diff/100001/runtime/vm/deopt_instru...
runtime/vm/deopt_instructions.cc:1136: ToCpuRegisterSource(source_loc));
4 spaces

https://codereview.chromium.org/504143003/diff/100001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler_ia32.cc (right):

https://codereview.chromium.org/504143003/diff/100001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler_ia32.cc:1580:
source.constant_instruction()->representation() == kUnboxedInt32) {
add parentheses

https://codereview.chromium.org/504143003/diff/100001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler_ia32.cc:1617:
source.constant_instruction()->representation() == kUnboxedInt32) {
add parentheses

https://codereview.chromium.org/504143003/diff/100001/runtime/vm/flow_graph_o...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/504143003/diff/100001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:626: deopt_id);
Why does the case (to == kUnboxedUin32 && from == kUnboxedInt32) not need
deopt-id (e.g., if from is negative)?

https://codereview.chromium.org/504143003/diff/100001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:4628: class DefinitionWorklist {
public ValueObject

https://codereview.chromium.org/504143003/diff/100001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:4658: BitVector* contains_;
I think it is slightly confusing to have contains() and Contains(Definition*
defn). Maybe rename contains_ to contains_vector_?

https://codereview.chromium.org/504143003/diff/100001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:4677: return true;
Why not kSHL?

https://codereview.chromium.org/504143003/diff/100001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:4697: if (smi_op != NULL &&
add parentheses

https://codereview.chromium.org/504143003/diff/100001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:4705: if (candidates.length() == 0) {
candidates.is_empty()

https://codereview.chromium.org/504143003/diff/100001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:4715: flow_graph()->loop_headers();
4 spaces indent

https://codereview.chromium.org/504143003/diff/100001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:4780: } else if (input->IsPhi() &&
input->Type()->ToCid() == kSmiCid) {
Add parentheses

https://codereview.chromium.org/504143003/diff/100001/runtime/vm/flow_graph_r...
File runtime/vm/flow_graph_range_analysis.h (right):

https://codereview.chromium.org/504143003/diff/100001/runtime/vm/flow_graph_r...
runtime/vm/flow_graph_range_analysis.h:430: }
I think it is confusing to have a non-static and static method with same name.

https://codereview.chromium.org/504143003/diff/100001/runtime/vm/intermediate...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/504143003/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:6939: #if defined(TARGET_ARCH_IA32) ||
defined(TARGET_ARCH_ARM)
You could move this into intermediate_language_xxx.cc

https://codereview.chromium.org/504143003/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:6993: ASSERT(idx == 0 || idx == 1);
parentheses

[Vyacheslav Egorov (Google), 2014-08-28 20:48:37]

DBC addressed

Please take a look at https://codereview.chromium.org/516013003/

https://codereview.chromium.org/504143003/diff/100001/runtime/vm/deopt_instru...
File runtime/vm/deopt_instructions.cc (right):

https://codereview.chromium.org/504143003/diff/100001/runtime/vm/deopt_instru...
runtime/vm/deopt_instructions.cc:1136: ToCpuRegisterSource(source_loc));
On 2014/08/27 16:17:12, srdjan wrote:
> 4 spaces

Done.

https://codereview.chromium.org/504143003/diff/100001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler_ia32.cc (right):

https://codereview.chromium.org/504143003/diff/100001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler_ia32.cc:1580:
source.constant_instruction()->representation() == kUnboxedInt32) {
On 2014/08/27 16:17:12, srdjan wrote:
> add parentheses

Done.

https://codereview.chromium.org/504143003/diff/100001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler_ia32.cc:1617:
source.constant_instruction()->representation() == kUnboxedInt32) {
On 2014/08/27 16:17:12, srdjan wrote:
> add parentheses

Done.

https://codereview.chromium.org/504143003/diff/100001/runtime/vm/flow_graph_o...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/504143003/diff/100001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:626: deopt_id);
On 2014/08/27 16:17:13, srdjan wrote:
> Why does the case (to == kUnboxedUin32 && from == kUnboxedInt32) not need
> deopt-id (e.g., if from is negative)?

Uint32 representation is kinda special right now in a sense that it is
*truncating* that is it just equivalent to doing implicit & 0xFFFFFFFF over its
input. That's why it *never* deopts.

https://codereview.chromium.org/504143003/diff/100001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:4628: class DefinitionWorklist {
On 2014/08/27 16:17:13, srdjan wrote:
> public ValueObject

Done.

https://codereview.chromium.org/504143003/diff/100001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:4658: BitVector* contains_;
On 2014/08/27 16:17:13, srdjan wrote:
> I think it is slightly confusing to have contains() and Contains(Definition*
> defn). Maybe rename contains_ to contains_vector_?

Done.

https://codereview.chromium.org/504143003/diff/100001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:4677: return true;
On 2014/08/27 16:17:13, srdjan wrote:
> Why not kSHL?

Shift Left by a *constant* does not need to untag or retag, so by itself it does
not benefit from Int32 representation right now.

https://codereview.chromium.org/504143003/diff/100001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:4697: if (smi_op != NULL &&
On 2014/08/27 16:17:13, srdjan wrote:
> add parentheses

Done.

https://codereview.chromium.org/504143003/diff/100001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:4705: if (candidates.length() == 0) {
On 2014/08/27 16:17:13, srdjan wrote:
> candidates.is_empty()

Done.

https://codereview.chromium.org/504143003/diff/100001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:4715: flow_graph()->loop_headers();
On 2014/08/27 16:17:13, srdjan wrote:
> 4 spaces indent

Done.

https://codereview.chromium.org/504143003/diff/100001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:4780: } else if (input->IsPhi() &&
input->Type()->ToCid() == kSmiCid) {
On 2014/08/27 16:17:13, srdjan wrote:
> Add parentheses

Done.

https://codereview.chromium.org/504143003/diff/100001/runtime/vm/flow_graph_r...
File runtime/vm/flow_graph_range_analysis.h (right):

https://codereview.chromium.org/504143003/diff/100001/runtime/vm/flow_graph_r...
runtime/vm/flow_graph_range_analysis.h:430: }
On 2014/08/27 16:17:13, srdjan wrote:
> I think it is confusing to have a non-static and static method with same name.

Agreed. Moved into a separate AllStatic helper class.

https://codereview.chromium.org/504143003/diff/100001/runtime/vm/intermediate...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/504143003/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:6939: #if defined(TARGET_ARCH_IA32) ||
defined(TARGET_ARCH_ARM)
On 2014/08/27 16:17:13, srdjan wrote:
> You could move this into intermediate_language_xxx.cc

Agreed. However I did not for two reasons:

1) This is a very temporary solution, and it is priority for me to actually
remove this #ifdef.

2) It would cause me to duplicated this switch twice (ARM, IA32) and duplicate
return false twice (MIPS, ARM64).

https://codereview.chromium.org/504143003/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:6993: ASSERT(idx == 0 || idx == 1);
On 2014/08/27 16:17:13, srdjan wrote:
> parentheses

Done.