Merge TRUNCDIV and MOD into one instruction. Icorporated feedback from CL https://codereview.chromi…

Merge TRUNCDIV and MOD into one instruction. Icorporated feedback from CL https://codereview.chromium.org/68663003

R=fschneider@google.com

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

[srdjan, 2013-11-20 22:23:27]

https://codereview.chromium.org/79653002/diff/1/runtime/vm/flow_graph_optimiz...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/79653002/diff/1/runtime/vm/flow_graph_optimiz...
runtime/vm/flow_graph_optimizer.cc:6925: // TODO(kmillikin): Handle conversion.
Just standard boiler-plate :-).

[Florian Schneider, 2013-11-21 11:15:41]

LGTM with comments.

https://codereview.chromium.org/79653002/diff/1/runtime/vm/flow_graph_optimiz...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/79653002/diff/1/runtime/vm/flow_graph_optimiz...
runtime/vm/flow_graph_optimizer.cc:311: BinarySmiOpInstr* other_binop =
(*merge_candidates)[k];
Can ohter_binop ever be NULL? I'm thinking of the case where there are 3 or more
merge candidates. Please add a test for these case as well:
3 candidates merged into 1 DIVMOD
4 or more candidates merged into 2 DIVMOD

https://codereview.chromium.org/79653002/diff/1/runtime/vm/flow_graph_optimiz...
runtime/vm/flow_graph_optimizer.cc:345: for_merge.Clear();
Maybe add a comment here or in TryMergeTruncDivMod that merging works on a
per-basic block basis.

https://codereview.chromium.org/79653002/diff/1/runtime/vm/flow_graph_optimiz...
runtime/vm/flow_graph_optimizer.cc:6925: // TODO(kmillikin): Handle conversion.
The comment still does not make sense since this is not a conversion.

https://codereview.chromium.org/79653002/diff/1/runtime/vm/intermediate_langu...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/79653002/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language.h:6583: static intptr_t
ArgumentCountFor(MergedMathInstr::Kind kind) {
We ususally use ArgumentCount for #arguments to a call. Maybe rename to
InputCountFor?

https://codereview.chromium.org/79653002/diff/1/runtime/vm/intermediate_langu...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/79653002/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_arm.cc:4091: deopt =
compiler->AddDeoptStub(deopt_id(), kDeoptBinarySmiOp);
s/kDeoptBinarySmiOp/kDeoptDivMod ?

[srdjan, 2013-11-21 17:32:43]

Ideally this optimization should happen after canonicalization. I will try this
in the following CL.

https://codereview.chromium.org/79653002/diff/1/runtime/vm/flow_graph_optimiz...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/79653002/diff/1/runtime/vm/flow_graph_optimiz...
runtime/vm/flow_graph_optimizer.cc:311: BinarySmiOpInstr* other_binop =
(*merge_candidates)[k];
On 2013/11/21 11:15:41, Florian Schneider wrote:
> Can ohter_binop ever be NULL? I'm thinking of the case where there are 3 or
more
> merge candidates. Please add a test for these case as well:
> 3 candidates merged into 1 DIVMOD
> 4 or more candidates merged into 2 DIVMOD 

Indeed it can.. fixed. Added tests as suggested

https://codereview.chromium.org/79653002/diff/1/runtime/vm/flow_graph_optimiz...
runtime/vm/flow_graph_optimizer.cc:345: for_merge.Clear();
On 2013/11/21 11:15:41, Florian Schneider wrote:
> Maybe add a comment here or in TryMergeTruncDivMod that merging works on a
> per-basic block basis.

Done.

https://codereview.chromium.org/79653002/diff/1/runtime/vm/flow_graph_optimiz...
runtime/vm/flow_graph_optimizer.cc:6925: // TODO(kmillikin): Handle conversion.
On 2013/11/21 11:15:41, Florian Schneider wrote:
> The comment still does not make sense since this is not a conversion.

  // TODO(srdjan): Handle merged instruction.

https://codereview.chromium.org/79653002/diff/1/runtime/vm/intermediate_langu...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/79653002/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language.h:6583: static intptr_t
ArgumentCountFor(MergedMathInstr::Kind kind) {
On 2013/11/21 11:15:41, Florian Schneider wrote:
> We ususally use ArgumentCount for #arguments to a call. Maybe rename to
> InputCountFor?

Done.

https://codereview.chromium.org/79653002/diff/1/runtime/vm/intermediate_langu...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/79653002/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_arm.cc:4091: deopt =
compiler->AddDeoptStub(deopt_id(), kDeoptBinarySmiOp);
On 2013/11/21 11:15:41, Florian Schneider wrote:
> s/kDeoptBinarySmiOp/kDeoptDivMod ?

It is the same deopt reason as it would happen for TRUNCDIV or MOD instruction,
which is kDeoptBinarySmiOp. So I would like to leave it as it is.

[srdjan, 2013-11-21 17:57:07]

Committed patchset #2 manually as r30536 (presubmit successful).