Enables per-function far-branches for ARM and MIPS.

Enables per-function far-branches for ARM and MIPS.

R=regis@google.com, srdjan@google.com

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

[zra, 2013-07-31 17:38:14]

Should I ask anyone else to review this?

[regis, 2013-07-31 18:01:12]

Yes, one of Srdjan, Florian, Slava, or Kevin should have a quick look at your
bailout code in runtime/vm/compiler.cc.

See my comment about the flag not belonging to the function.

The assembler changes LGTM.

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

https://codereview.chromium.org/21363003/diff/1/runtime/vm/assembler_arm.cc#n...
runtime/vm/assembler_arm.cc:1734: if
(!Utils::IsInt(Utils::CountOneBits(kBranchOffsetMask), offset)) {
ASSERT(!use_far_branches());
ditto for MIPS

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

https://codereview.chromium.org/21363003/diff/1/runtime/vm/assembler_ia32.h#n...
runtime/vm/assembler_ia32.h:315: ~Assembler() { }
You could add an ASSERT(!use_far_branches) with a comment that this mode is not
supported/necessary on IA32 (ditto for X64).

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

https://codereview.chromium.org/21363003/diff/1/runtime/vm/compiler.cc#newcod...
runtime/vm/compiler.cc:497: Assembler assembler(function.needs_far_branches());
I think that the property of requiring far branches belongs to the flow graph,
rather than to the function. I can imagine that the unoptimized version of a
function does not require far branches, whereas the optimized one, with
inlining, does require it. The opposite could be true too, with the optimized
version being more compact.

[zra, 2013-07-31 18:05:19]

+srdjan.

[zra, 2013-07-31 20:10:51]

Thanks. I've made these changes. I'm still tracking down a bug, but it looks
like it might just be a find-replace error somewhere.

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

https://codereview.chromium.org/21363003/diff/1/runtime/vm/assembler_arm.cc#n...
runtime/vm/assembler_arm.cc:1734: if
(!Utils::IsInt(Utils::CountOneBits(kBranchOffsetMask), offset)) {
On 2013/07/31 18:01:12, regis wrote:
> ASSERT(!use_far_branches());
> ditto for MIPS

Done.

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

https://codereview.chromium.org/21363003/diff/1/runtime/vm/assembler_ia32.h#n...
runtime/vm/assembler_ia32.h:315: ~Assembler() { }
On 2013/07/31 18:01:12, regis wrote:
> You could add an ASSERT(!use_far_branches) with a comment that this mode is
not
> supported/necessary on IA32 (ditto for X64).

Done.

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

https://codereview.chromium.org/21363003/diff/1/runtime/vm/compiler.cc#newcod...
runtime/vm/compiler.cc:497: Assembler assembler(function.needs_far_branches());
On 2013/07/31 18:01:12, regis wrote:
> I think that the property of requiring far branches belongs to the flow graph,
> rather than to the function. I can imagine that the unoptimized version of a
> function does not require far branches, whereas the optimized one, with
> inlining, does require it. The opposite could be true too, with the optimized
> version being more compact.

That makes sense to me. I've moved the flag to the FlowGraph.

[srdjan, 2013-07-31 22:52:27]

https://codereview.chromium.org/21363003/diff/11001/runtime/vm/assembler.cc
File runtime/vm/assembler.cc (left):

https://codereview.chromium.org/21363003/diff/11001/runtime/vm/assembler.cc#o...
runtime/vm/assembler.cc:21: DEFINE_FLAG(bool, use_far_branches, false, "Enable
far branches");
I do not think there is any harm defining this flag in machine independent code
(description should say ARM and MIPS only).

https://codereview.chromium.org/21363003/diff/11001/runtime/vm/assembler_arm.h
File runtime/vm/assembler_arm.h (right):

https://codereview.chromium.org/21363003/diff/11001/runtime/vm/assembler_arm....
runtime/vm/assembler_arm.h:739: bool use_far_branches_;
const

https://codereview.chromium.org/21363003/diff/11001/runtime/vm/assembler_ia32.h
File runtime/vm/assembler_ia32.h (right):

https://codereview.chromium.org/21363003/diff/11001/runtime/vm/assembler_ia32...
runtime/vm/assembler_ia32.h:316: ASSERT(!use_far_branches);
I would not assert at all, does not matter if we pass true or false.

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

https://codereview.chromium.org/21363003/diff/11001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.h:1100: bool use_far_branches_;
const

https://codereview.chromium.org/21363003/diff/11001/runtime/vm/assembler_x64.h
File runtime/vm/assembler_x64.h (right):

https://codereview.chromium.org/21363003/diff/11001/runtime/vm/assembler_x64....
runtime/vm/assembler_x64.h:330: ASSERT(!use_far_branches);
No Assert

[zra, 2013-08-01 00:28:32]

Thanks! I've changed the bailout loop as we discussed. Please have another look.

https://codereview.chromium.org/21363003/diff/11001/runtime/vm/assembler.cc
File runtime/vm/assembler.cc (left):

https://codereview.chromium.org/21363003/diff/11001/runtime/vm/assembler.cc#o...
runtime/vm/assembler.cc:21: DEFINE_FLAG(bool, use_far_branches, false, "Enable
far branches");
On 2013/07/31 22:52:27, srdjan wrote:
> I do not think there is any harm defining this flag in machine independent
code
> (description should say ARM and MIPS only).

Added back

https://codereview.chromium.org/21363003/diff/11001/runtime/vm/assembler_arm.h
File runtime/vm/assembler_arm.h (right):

https://codereview.chromium.org/21363003/diff/11001/runtime/vm/assembler_arm....
runtime/vm/assembler_arm.h:739: bool use_far_branches_;
On 2013/07/31 22:52:27, srdjan wrote:
> const

Done.

https://codereview.chromium.org/21363003/diff/11001/runtime/vm/assembler_ia32.h
File runtime/vm/assembler_ia32.h (right):

https://codereview.chromium.org/21363003/diff/11001/runtime/vm/assembler_ia32...
runtime/vm/assembler_ia32.h:316: ASSERT(!use_far_branches);
On 2013/07/31 22:52:27, srdjan wrote:
> I would not assert at all, does not matter if we pass true or false.

I like it as a check that the loop/logic in the compiler in
CompileParsedFunctionHelper is correct. For ARM and MIPS, If there is a logic
error, I could be passing true all the time and never notice, but then if I run
on ia32, It'll fail immediately.

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

https://codereview.chromium.org/21363003/diff/11001/runtime/vm/assembler_mips...
runtime/vm/assembler_mips.h:1100: bool use_far_branches_;
On 2013/07/31 22:52:27, srdjan wrote:
> const

Done.

[zra, 2013-08-01 17:46:20]

Some performance numbers from hardware:

DeltaBlue before change:
$ out/ReleaseARM/dart benchmarks/DeltaBlue/dart/DeltaBlue.dart 
DeltaBlue(RunTime): 3885.4368932038833 us.

Before change with far-branches used in every function
$ out/ReleaseARM/dart --use-far-branches
benchmarks/DeltaBlue/dart/DeltaBlue.dart 
DeltaBlue(RunTime): 4476.5100671140935 us.

After this change (far-branches used only in functions that need them):
$ out/ReleaseARM/dart benchmarks/DeltaBlue/dart/DeltaBlue.dart 
DeltaBlue(RunTime): 3883.4951456310678 us.

As expected since DeltaBlue doesn't need far-branches anywhere.

[srdjan, 2013-08-01 19:59:13]

lgtm

https://codereview.chromium.org/21363003/diff/32001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/21363003/diff/32001/runtime/vm/compiler.cc#ne...
runtime/vm/compiler.cc:256: // static to evade gcc's longjmp variable smashing
checks.
Maybe add a comment explaining that use_far_branches can be true for ARM and
MIPS only,

[zra, 2013-08-01 20:16:31]

Thanks!

https://codereview.chromium.org/21363003/diff/32001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/21363003/diff/32001/runtime/vm/compiler.cc#ne...
runtime/vm/compiler.cc:256: // static to evade gcc's longjmp variable smashing
checks.
On 2013/08/01 19:59:13, srdjan wrote:
> Maybe add a comment explaining that use_far_branches can be true for ARM and
> MIPS only,

Done.

[zra, 2013-08-01 20:51:55]

Committed patchset #5 manually as r25723 (presubmit successful).

[Ivan Posva, 2013-08-06 00:17:51]

This threading issue needs to be addressed.

-Ivan

https://codereview.chromium.org/21363003/diff/45001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/21363003/diff/45001/runtime/vm/compiler.cc#ne...
runtime/vm/compiler.cc:264: static bool use_far_branches = false;
How is this supposed to work with multiple threads?

[Florian Schneider, 2013-08-07 09:21:26]

https://codereview.chromium.org/21363003/diff/45001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/21363003/diff/45001/runtime/vm/compiler.cc#ne...
runtime/vm/compiler.cc:264: static bool use_far_branches = false;
On 2013/08/06 00:17:51, Ivan Posva wrote:
> How is this supposed to work with multiple threads?

It should rather be

volatile bool use_far_branches = false;

[zra, 2013-08-09 15:19:43]

https://codereview.chromium.org/21363003/diff/45001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/21363003/diff/45001/runtime/vm/compiler.cc#ne...
runtime/vm/compiler.cc:264: static bool use_far_branches = false;
On 2013/08/06 00:17:51, Ivan Posva wrote:
> How is this supposed to work with multiple threads?

Sorry, forgot about threads. You're right this won't work.

https://codereview.chromium.org/21363003/diff/45001/runtime/vm/compiler.cc#ne...
runtime/vm/compiler.cc:264: static bool use_far_branches = false;
On 2013/08/07 09:21:26, Florian Schneider wrote:
> On 2013/08/06 00:17:51, Ivan Posva wrote:
> > How is this supposed to work with multiple threads?
> 
> It should rather be
> 
> volatile bool use_far_branches = false;

Yes. Thanks for the fix. I have sent you and Ivan a CL for review with this
change.