Pattern match on generated code to find edge counters.

Pattern match on generated code to find edge counters.

In unoptimized code, use platform-specific pattern matching on generated
code to find edge counter arrays.  Previously we searched pointer offsets,
but that does not work on platforms that encode the edge counters as indexes
into an object pool (i.e., x64, ARM, MIPS).

BUG=
R=fschneider@google.com, zra@google.com

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

[Kevin Millikin (Google), 2013-09-26 11:44:52]

This change should reenable block reordering on x64 and fix it on ARM and MIPS
where it's broken.  There are several bug fixes needed:

1. Add missing edge counters to GotoInstr on ARM and MIPS.
2. Stop recording duplicate deoptimization descriptors for GotoInstr on all
platforms.

[srdjan, 2013-09-26 18:37:08]

DBC:

Instead of pattern matching, you could use PCDescriptors to mark counter
locations. I think that would simplify the code quite a bit.

[Kevin Millikin (Google), 2013-09-27 09:52:04]

On 2013/09/26 18:37:08, srdjan wrote:
> DBC:
> 
> Instead of pattern matching, you could use PCDescriptors to mark counter
> locations. I think that would simplify the code quite a bit.

I'm not so sure.  Most of the complexity here is due to refactoring some class
member functions on CallPattern on ARM (which don't have anything specifically
to do with call patterns) so that they are static helpers.  On ia32, x64, and
mips the pattern matching approach is dead simple.

If we used the descriptors, we'd have to add a new descriptor type and still
have to deal with the four possible different patterns on ARM (via pattern
matching?).  So that seems like the same or more complexity.  Plus, another
descriptor type makes it harder to compress the (bloated) descriptors in the
future, and I think that that day is coming sooner rather than later.

Florian, what do you think?

[Florian Schneider, 2013-09-30 08:55:38]

Space cost is an issue. Considering that we already have the instruction
decoding logic in, I'd use that code as well for now. I agree it is a pain to
keep the pattern matching code in sync. It should not change too frequently.

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

https://codereview.chromium.org/24744002/diff/1/runtime/vm/code_patcher_arm.c...
runtime/vm/code_patcher_arm.cc:98: // will partial validity.
missing verb in this comment?

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

https://codereview.chromium.org/24744002/diff/1/runtime/vm/code_patcher_mips....
runtime/vm/code_patcher_mips.cc:111: int offset = Instr::At(start)->SImmField()
+ kHeapObjectTag;
What about indices in the constant pool greater than the maximum immediate size
(kImmbits=16)?

I think the assember will emit a multiple instruction (like on ARM) in that
case.

[Kevin Millikin (Google), 2013-09-30 09:16:17]

> I think the assember will emit a multiple instruction (like on ARM) in that
> case.

You are right, that needs to be fixed.  I'll also move the deoptimization
descriptor to after the counter for both TargetEntry and Goto on all platforms,
for uniformity.

[Kevin Millikin (Google), 2013-09-30 13:59:35]

I've refactored this:

1. Emit the deoptimization descriptor after the edge counters on all platforms.

2. Pattern match correctly on MIPS.

3. Skip the overflow check, it's not likely to be necessary and it's safe to
skip it.

4. Stop duplicating the edge counter code.

Regis, Zach, could you take a look at the ARM and MIPS code in
FlowGraphCompiler::EmitEdgeCounter() and make sure it's OK?

[zra, 2013-09-30 15:22:14]

arm/mips in FlowGraphCompiler::EmitEdgeCounter() lgtm.

https://codereview.chromium.org/24744002/diff/9001/runtime/vm/code_patcher_mi...
File runtime/vm/code_patcher_mips.cc (right):

https://codereview.chromium.org/24744002/diff/9001/runtime/vm/code_patcher_mi...
runtime/vm/code_patcher_mips.cc:89: // MIPS is complicated because it can take
four possible different forms.
s/four/two/

https://codereview.chromium.org/24744002/diff/9001/runtime/vm/flow_graph_comp...
File runtime/vm/flow_graph_compiler_arm.cc (right):

https://codereview.chromium.org/24744002/diff/9001/runtime/vm/flow_graph_comp...
runtime/vm/flow_graph_compiler_arm.cc:1267: __ adds(IP, IP,
ShifterOperand(Smi::RawValue(1)));
Maybe just add() if you don't need to set the condition flags.

[sra1, 2013-09-30 16:23:06]

https://codereview.chromium.org/24744002/diff/9001/runtime/vm/code_patcher_x6...
File runtime/vm/code_patcher_x64.cc (right):

https://codereview.chromium.org/24744002/diff/9001/runtime/vm/code_patcher_x6...
runtime/vm/code_patcher_x64.cc:274: //     4c 01 58 17                  addq
[rax+0x17],r11
This is pattern matching the poor encoding of addq.
Perhaps it would beworth fixing addq first
See http://dartbug.com/13681

[Florian Schneider, 2013-10-01 10:10:01]

lgtm

https://codereview.chromium.org/24744002/diff/9001/runtime/vm/intermediate_la...
File runtime/vm/intermediate_language_mips.cc (right):

https://codereview.chromium.org/24744002/diff/9001/runtime/vm/intermediate_la...
runtime/vm/intermediate_language_mips.cc:3871: 0);  // No token position.
Use Scanner::kDummyTokenIndex?

[Kevin Millikin (Google), 2013-10-01 10:23:19]

Committed patchset #3 manually as r28085 (presubmit successful).