Reduce copying, redundancy & repetition for codegen of comparison instructions

Reduce copying, redundancy & repetition for codegen of comparison instructions

R=vegorov@google.com
BUG=

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

[Vyacheslav Egorov (Google), 2017-06-15 11:32:47]

lgtm

https://codereview.chromium.org/2937933002/diff/1/runtime/vm/assembler_mips.h
File runtime/vm/assembler_mips.h (right):

https://codereview.chromium.org/2937933002/diff/1/runtime/vm/assembler_mips.h...
runtime/vm/assembler_mips.h:180: void SetToInvalidState() {
Maybe this method should be private?

https://codereview.chromium.org/2937933002/diff/1/runtime/vm/assembler_mips.h...
runtime/vm/assembler_mips.h:187: Condition() : ValueObject(), bits_(0) {}
I would suggest that this one sets bits_ to invalid relation.

https://codereview.chromium.org/2937933002/diff/1/runtime/vm/assembler_mips.h...
runtime/vm/assembler_mips.h:209: if (rel_op == INVALID_RELATION) {
... and here ASSERT(rel_op != INVALID_RELATION)

https://codereview.chromium.org/2937933002/diff/1/runtime/vm/assembler_mips.h...
runtime/vm/assembler_mips.h:218: static bool IsValidRelOp(RelationOperator
value) {
I would not be surprised if this gets completely optimized out - it is UB to
convert out-of-bounds values to enum.

https://codereview.chromium.org/2937933002/diff/1/runtime/vm/constants_arm.h
File runtime/vm/constants_arm.h (right):

https://codereview.chromium.org/2937933002/diff/1/runtime/vm/constants_arm.h#...
runtime/vm/constants_arm.h:358: kInvalidCondition = 16
Maybe call it INVALID_CONDITION on all platforms for consistency?

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

https://codereview.chromium.org/2937933002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.h:2443: virtual void
EmitBranchCode(FlowGraphCompiler* compiler, BranchInstr* branch);
I think all of these methods should simply go into ComparisonInstr - then you
don't need explicit template instantiations for different variants of
EmitBranchCode.

https://codereview.chromium.org/2937933002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.h:2443: virtual void
EmitBranchCode(FlowGraphCompiler* compiler, BranchInstr* branch);
Also comment what this does.

https://codereview.chromium.org/2937933002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.h:2448: // a branch based on that condition. 
May also branch directly to the labels.
"emit a branch based on that condition" <- specify where the branch can go (e.g.
should it go to false or true?)

https://codereview.chromium.org/2937933002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.h:2453: virtual Condition
GetPredictedCondition(FlowGraphCompiler* compiler,
Comment what this does.

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

https://codereview.chromium.org/2937933002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_arm64.cc:5874: // TODO(erikcorry): Use csel
here for better branch prediction?
File a bug and put it into a TODO(dartbug.com/...). The bug should provide
background for the TODO.

We have way too many named TODOs nobody remembers about.

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

https://codereview.chromium.org/2937933002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_dbc.cc:486: Condition
StrictCompareInstr::GetPredictedCondition(FlowGraphCompiler* compiler,
The name GetPredicatedCondition(...) is very confusing. 

Maybe it should be simply called GetNextInstructioCondition(...) or something
like that.

https://codereview.chromium.org/2937933002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_dbc.cc:532: return condition;
this should probably be return GetPredictedCondition(...).

https://codereview.chromium.org/2937933002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_dbc.cc:668: return (kind() == Token::kEQ) ?
NEXT_IS_TRUE : NEXT_IS_FALSE;
should this get return GetPredictedCondition()?

https://codereview.chromium.org/2937933002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_dbc.cc:1801: const bool is_negated = kind() !=
Token::kEQ;
return GetPredictedCondition();

[erikcorry, 2017-06-19 07:15:09]

https://codereview.chromium.org/2937933002/diff/1/runtime/vm/assembler_mips.h
File runtime/vm/assembler_mips.h (right):

https://codereview.chromium.org/2937933002/diff/1/runtime/vm/assembler_mips.h...
runtime/vm/assembler_mips.h:180: void SetToInvalidState() {
On 2017/06/15 11:32:46, Vyacheslav Egorov (Google) wrote:
> Maybe this method should be private?

Done.

https://codereview.chromium.org/2937933002/diff/1/runtime/vm/assembler_mips.h...
runtime/vm/assembler_mips.h:187: Condition() : ValueObject(), bits_(0) {}
On 2017/06/15 11:32:46, Vyacheslav Egorov (Google) wrote:
> I would suggest that this one sets bits_ to invalid relation.

Done.

https://codereview.chromium.org/2937933002/diff/1/runtime/vm/assembler_mips.h...
runtime/vm/assembler_mips.h:209: if (rel_op == INVALID_RELATION) {
On 2017/06/15 11:32:46, Vyacheslav Egorov (Google) wrote:
> ... and here ASSERT(rel_op != INVALID_RELATION)

I put the assert in the getters for the register numbers.

https://codereview.chromium.org/2937933002/diff/1/runtime/vm/assembler_mips.h...
runtime/vm/assembler_mips.h:218: static bool IsValidRelOp(RelationOperator
value) {
On 2017/06/15 11:32:46, Vyacheslav Egorov (Google) wrote:
> I would not be surprised if this gets completely optimized out - it is UB to
> convert out-of-bounds values to enum. 

Ack

https://codereview.chromium.org/2937933002/diff/1/runtime/vm/constants_arm.h
File runtime/vm/constants_arm.h (right):

https://codereview.chromium.org/2937933002/diff/1/runtime/vm/constants_arm.h#...
runtime/vm/constants_arm.h:358: kInvalidCondition = 16
On 2017/06/15 11:32:46, Vyacheslav Egorov (Google) wrote:
> Maybe call it INVALID_CONDITION on all platforms for consistency?

OK, but then I have to rename kNumberOfConditions and kSpecialCondition too,
otherwise it looks strange.  We agreed offline to leave it like this.

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

https://codereview.chromium.org/2937933002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.h:2443: virtual void
EmitBranchCode(FlowGraphCompiler* compiler, BranchInstr* branch);
On 2017/06/15 11:32:46, Vyacheslav Egorov (Google) wrote:
> I think all of these methods should simply go into ComparisonInstr - then you
> don't need explicit template instantiations for different variants of
> EmitBranchCode.

I was sure there was some reason that would not work, but it works. :-)

https://codereview.chromium.org/2937933002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.h:2443: virtual void
EmitBranchCode(FlowGraphCompiler* compiler, BranchInstr* branch);
On 2017/06/15 11:32:46, Vyacheslav Egorov (Google) wrote:
> Also comment what this does.

Done.

https://codereview.chromium.org/2937933002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.h:2448: // a branch based on that condition. 
May also branch directly to the labels.
On 2017/06/15 11:32:46, Vyacheslav Egorov (Google) wrote:
> "emit a branch based on that condition" <- specify where the branch can go
(e.g.
> should it go to false or true?)

Done.

https://codereview.chromium.org/2937933002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.h:2453: virtual Condition
GetPredictedCondition(FlowGraphCompiler* compiler,
On 2017/06/15 11:32:46, Vyacheslav Egorov (Google) wrote:
> Comment what this does.

Done.

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

https://codereview.chromium.org/2937933002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_arm64.cc:5874: // TODO(erikcorry): Use csel
here for better branch prediction?
On 2017/06/15 11:32:46, Vyacheslav Egorov (Google) wrote:
> File a bug and put it into a TODO(dartbug.com/...). The bug should provide
> background for the TODO.
> 
> We have way too many named TODOs nobody remembers about.

Done.

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

https://codereview.chromium.org/2937933002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_dbc.cc:486: Condition
StrictCompareInstr::GetPredictedCondition(FlowGraphCompiler* compiler,
On 2017/06/15 11:32:46, Vyacheslav Egorov (Google) wrote:
> The name GetPredicatedCondition(...) is very confusing. 
> 
> Maybe it should be simply called GetNextInstructioCondition(...) or something
> like that.

Done.

https://codereview.chromium.org/2937933002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_dbc.cc:532: return condition;
On 2017/06/15 11:32:46, Vyacheslav Egorov (Google) wrote:
> this should probably be return GetPredictedCondition(...).

We assert that they return the same. I think that's better because it will
trigger if this is changed without fixing the GetPredictedCondition.

https://codereview.chromium.org/2937933002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_dbc.cc:668: return (kind() == Token::kEQ) ?
NEXT_IS_TRUE : NEXT_IS_FALSE;
On 2017/06/15 11:32:46, Vyacheslav Egorov (Google) wrote:
> should this get return GetPredictedCondition()?

See above.

https://codereview.chromium.org/2937933002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_dbc.cc:1801: const bool is_negated = kind() !=
Token::kEQ;
On 2017/06/15 11:32:46, Vyacheslav Egorov (Google) wrote:
> return GetPredictedCondition();

See above

[erikcorry, 2017-06-19 07:45:10]

Description was changed from

==========
Reduce copying, redundancy & repetition for codegen of comparison instructions

R=vegorov@google.com
BUG=
==========

to

==========
Reduce copying, redundancy & repetition for codegen of comparison instructions

R=vegorov@google.com
BUG=

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

[erikcorry, 2017-06-19 07:45:11]

Committed patchset #2 (id:10001) manually as
e25dfc07d193246eb30eedf085141e9f65e98b06 (presubmit successful).