Streamline code generator for branches and comparisons.

Streamline code generator for branches and comparisons.

This is another refactoring step. It turn all comparisons
 to be generated after the same pattern and removes duplicated
 code from each of the different comparisons (smi, mint, double, etc.)

A comparison as an expression now follows the same patterns as a comparison
inside a branch. It just adds a common epilogue to materialize the boolean
result.

In another CL this common epilogue will be generated by reusing the IfThenElse
IL instruction.

R=kmillikin@google.com

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

[Florian Schneider, 2013-11-07 14:14:08]

This is ia32 only. I'll port this after addressing comments for this approach.

[Kevin Millikin (Google), 2013-11-07 14:34:08]

Nice simplification.  I like the approach, didn't double check super closely yet
for correctness.

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

https://codereview.chromium.org/64483002/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_ia32.cc:339: if (value && labels.fall_through
!= labels.true_label) {
I've actually gotten used to the extra parens around the != comparison.  Put
them here and below.

https://codereview.chromium.org/64483002/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_ia32.cc:532: Label* fall_through =
compiler->CanFallThroughTo(branch->true_successor())
Hmmm, that's kind of ugly.  What about a function to get the label of the next
non-empty block:

Label* FlowGraphCompiler::NextNonemptyLabel(BlockEntryInstr* block_entry) const
{
  const intptr_t current_index = current_block()->postorder_number();
  return block_info_[current_index]->next_nonempty_label();
}

And set fall_through to that --- it doesn't have to be NULL, just some pointer
distinct from true_label and false_label.  Or do you think that's too
error-prone?

[Florian Schneider, 2013-11-08 12:00:20]

I'll go ahead and add the other platforms.

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

https://codereview.chromium.org/64483002/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_ia32.cc:339: if (value && labels.fall_through
!= labels.true_label) {
On 2013/11/07 14:34:09, kmillikin wrote:
> I've actually gotten used to the extra parens around the != comparison.  Put
> them here and below.

Done.

https://codereview.chromium.org/64483002/diff/1/runtime/vm/intermediate_langu...
runtime/vm/intermediate_language_ia32.cc:532: Label* fall_through =
compiler->CanFallThroughTo(branch->true_successor())
On 2013/11/07 14:34:09, kmillikin wrote:
> Hmmm, that's kind of ugly.  What about a function to get the label of the next
> non-empty block:
> 
> Label* FlowGraphCompiler::NextNonemptyLabel(BlockEntryInstr* block_entry)
const
> {
>   const intptr_t current_index = current_block()->postorder_number();
>   return block_info_[current_index]->next_nonempty_label();
> }
> 
> And set fall_through to that --- it doesn't have to be NULL, just some pointer
> distinct from true_label and false_label.  Or do you think that's too
> error-prone?

Agreed. CanFallThroughTo uses next_nonempty_label anyway, so it is as good.

[Florian Schneider, 2013-11-11 13:30:41]

Added other plaforms. This CL does not change the generated code for any of the
affected instructions.

https://codereview.chromium.org/64483002/diff/70001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/64483002/diff/70001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_arm.cc:522: __ LoadObject(result, Bool::True(),
true_condition);
The ARM code differs slightly from the other platform because it uses
conditional-move instead of branches for the smi-comparison case.

[Kevin Millikin (Google), 2013-11-13 17:44:24]

Yes, lgtm.

[Florian Schneider, 2013-11-13 18:21:38]

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