Initial steps towards cleaning up integer arithmetic IR.

Initial steps towards cleaning up integer arithmetic IR.

Introduce base classes for binary and unary integer operations and move all shared logic (e.g. canonicalization) into these classes.

Start removing special cases dispatch from instructions patterns - it goes into Canonicalization pass.

Start removing duplication between instruction patterns where meaningful and possible.

Cleanup the notion of truncation in the operations: is_truncating implies !can_overflow, not the other way around.

BUG=
R=johnmccutchan@google.com, srdjan@google.com

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

[Vyacheslav Egorov (Google), 2014-09-11 15:35:30]

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

[Vyacheslav Egorov (Google), 2014-09-11 15:36:27]

PTAL

This is first steps, I did not complete the cleanup that I planned because I
have other more urgent matters, but I don't want to sit on this change list and
wait til it rots :)

[srdjan, 2014-09-11 17:38:12]

lgtm

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

https://codereview.chromium.org/564843002/diff/1/runtime/vm/flow_graph.cc#new...
runtime/vm/flow_graph.cc:98: constant = new(isolate())
ConstantInstr(Object::ZoneHandle(object.raw()));
isolate(), object.raw()

https://codereview.chromium.org/564843002/diff/1/runtime/vm/flow_graph_range_...
File runtime/vm/flow_graph_range_analysis.cc (right):

https://codereview.chromium.org/564843002/diff/1/runtime/vm/flow_graph_range_...
runtime/vm/flow_graph_range_analysis.cc:2017: Range::IsUnknown(right_range)) {
One line?

https://codereview.chromium.org/564843002/diff/1/runtime/vm/intermediate_lang...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/564843002/diff/1/runtime/vm/intermediate_lang...
runtime/vm/intermediate_language.cc:307: BinaryIntegerOpInstr* other_op =
static_cast<BinaryIntegerOpInstr*>(other);
other->AsBinary... ?

https://codereview.chromium.org/564843002/diff/1/runtime/vm/intermediate_lang...
runtime/vm/intermediate_language.cc:1251: 
Two lines instead of three

https://codereview.chromium.org/564843002/diff/1/runtime/vm/intermediate_lang...
runtime/vm/intermediate_language.cc:1402: static_cast<uint64_t>(-1) >> (64 -
RepresentationBits(r)));
4 spaces indent

https://codereview.chromium.org/564843002/diff/1/runtime/vm/intermediate_lang...
runtime/vm/intermediate_language.cc:1435: }
Instead you could add return NULL to default (remove UNREACHABLE)

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

https://codereview.chromium.org/564843002/diff/1/runtime/vm/intermediate_lang...
runtime/vm/intermediate_language.h:6720: return
static_cast<UnaryIntegerOpInstr*>(other)->op_kind() == op_kind();
Why static_cast instead of other->AsUnary... ?

[Cutch, 2014-09-11 17:41:52]

johnmccutchan@google.com changed reviewers:
+ johnmccutchan@google.com

[Cutch, 2014-09-11 17:41:53]

lgtm

https://codereview.chromium.org/564843002/diff/1/runtime/vm/flow_graph_optimi...
File runtime/vm/flow_graph_optimizer.cc (left):

https://codereview.chromium.org/564843002/diff/1/runtime/vm/flow_graph_optimi...
runtime/vm/flow_graph_optimizer.cc:2253: call->token_pos());
Why are we dropping the token position?

https://codereview.chromium.org/564843002/diff/1/runtime/vm/intermediate_lang...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/564843002/diff/1/runtime/vm/intermediate_lang...
runtime/vm/intermediate_language.cc:1416: 
remove blank lines

https://codereview.chromium.org/564843002/diff/1/runtime/vm/intermediate_lang...
runtime/vm/intermediate_language.cc:1459: 
ditto

https://codereview.chromium.org/564843002/diff/1/runtime/vm/intermediate_lang...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/564843002/diff/1/runtime/vm/intermediate_lang...
runtime/vm/intermediate_language_ia32.cc:2890: // const int32_t imm =
reinterpret_cast<int32_t>(constant.raw());
kill

https://codereview.chromium.org/564843002/diff/1/runtime/vm/intermediate_lang...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/564843002/diff/1/runtime/vm/intermediate_lang...
runtime/vm/intermediate_language_x64.cc:2771: if (deopt != NULL) __ j(OVERFLOW,
deopt);
Please use curlies (here and elsewhere):

if (deopt != NULL) {
  __ j(OVERFLOW, deopt);
}

[Vyacheslav Egorov (Google), 2014-09-11 19:50:47]

some answers below, I'll address comments and land.

thanks for the review.

https://codereview.chromium.org/564843002/diff/1/runtime/vm/flow_graph_optimi...
File runtime/vm/flow_graph_optimizer.cc (left):

https://codereview.chromium.org/564843002/diff/1/runtime/vm/flow_graph_optimi...
runtime/vm/flow_graph_optimizer.cc:2253: call->token_pos());
On 2014/09/11 17:41:53, Cutch wrote:
> Why are we dropping the token position?

Because it was not used anywhere. Remains of ancient times.

https://codereview.chromium.org/564843002/diff/1/runtime/vm/intermediate_lang...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/564843002/diff/1/runtime/vm/intermediate_lang...
runtime/vm/intermediate_language.cc:1435: }
On 2014/09/11 17:38:11, srdjan wrote:
> Instead you could add return NULL to default (remove UNREACHABLE)

I would like to ensure that we don't forget to update this place when we
introduce new integer representation (if ever :))

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

https://codereview.chromium.org/564843002/diff/1/runtime/vm/intermediate_lang...
runtime/vm/intermediate_language.h:6720: return
static_cast<UnaryIntegerOpInstr*>(other)->op_kind() == op_kind();
On 2014/09/11 17:38:12, srdjan wrote:
> Why static_cast instead of other->AsUnary... ?

That would require manually introducing AsUnary... as it is not a real
instruction but an abstract class, but we don't have As... methods for those.

I will do this. 

Though I think we should change the way As... operators are implemented.
Instruction now has a gigantic vtable for now good reason because of that.

[Vyacheslav Egorov (Google), 2014-09-17 13:31:38]

Committed patchset #1 (id:1) manually as 40375 (presubmit successful).