ARM Skia NEON patches - 35 - First AArch64 support

ARM Skia NEON patches - 35 - First AArch64 support

Aarch64 support

This change contains the necessary modifications to have Skia build and
run properly on an ARMv8 processor in aarch64 execution state.

Here's a list of the changes:

 - add an arm64 target to the build system + SK_CPU_ARM64 flag

 - MatrixTest was failing when built in Release mode. Fused MAC
   instructions were generated which made some intermediate results
   more accurate. As the test relies on result comparison, the more
   precise results when compared to others led to a gap bigger than
   what was tolerated. As I don't know if some actual skia code relies
   on results being comparable, I've disabled fused MAC instruction
   with -ffp-contract=off for arm64.

 - Modify include/core/SkOnce.h to have barriers work.

 - SK_CPU_ARM64 implies SK_ARM_NEON_MODE_ALWAYS.

 - use existing Xfermode optimisations with modifications that can be
   removed in the future when toolchains are ready. Also save a few
   instructions is two Xfermodes (will apply to ARM too).

 - use existing SkBoxBlur and SkMorphology optimisations.

 - use existing SkBlitMask optimisations

 - use existing BitmapProcState and Convolution optimisations.

Future changes will include:

 - Blitters (only partialy merged upstream)

 - SkUtils (there's little value in sending asm optimisations without
   having them benchmarked on real hardware).

Signed-off-by: Kevin PETIT <kevin.petit@arm.com>;

BUG=skia:

Committed: http://code.google.com/p/skia/source/detail?r=13980

Committed: http://code.google.com/p/skia/source/detail?r=14025

[hal.canary, 2014-03-18 16:01:07]

Should arm64 be another skia_arch_type, or should we leave it skia_arch_type=arm
but with skia_arch_width=64?  That would be consistent with how x86 is handled.

[kevin.petit, 2014-03-18 16:22:56]

On 2014/03/18 16:01:07, Hal Canary wrote:
> Should arm64 be another skia_arch_type, or should we leave it
skia_arch_type=arm
> but with skia_arch_width=64?  That would be consistent with how x86 is
handled.

I'm tempted to push for a different skia_arch_type. x86 and x86_64 share a
certain degree of compatibility.

arm64 (AArch64 to be precise) shares much less with v7 or AArch32. For a start
the instruction set is not the same.

[hal.canary, 2014-03-18 16:24:28]

Kevin,

Which GYP_DEFINES were you passing to gyp_skia to test this?

[hal.canary, 2014-03-18 16:30:01]

On 2014/03/18 16:24:28, Hal Canary wrote:
> Kevin,
> 
> Which GYP_DEFINES were you passing to gyp_skia to test this?

I figured it out.  I'm using: 

  skia_giflib_static=1
  skia_libpng_static=1
  skia_zlib_static=1
  skia_freetype_static=1
  skia_no_fontconfig=1
  skia_poppler_enabled=0
  skia_skip_gui=1
  skia_gpu=0
  skia_arch_type=arm64
  skia_arch_width=64
  arm_thumb=1

for testing your patch.

[kevin.petit, 2014-03-18 16:30:10]

On 2014/03/18 16:24:28, Hal Canary wrote:
> Kevin,
> 
> Which GYP_DEFINES were you passing to gyp_skia to test this?

export GYP_DEFINES="skia_arch_type=arm64 linux_use_tcmalloc=0 skia_gpu=0
skia_warnings_as_errors=0" is what I've used the most.

[kevin.petit, 2014-03-18 16:32:29]

On 2014/03/18 16:30:01, Hal Canary wrote:
> On 2014/03/18 16:24:28, Hal Canary wrote:
> > Kevin,
> > 
> > Which GYP_DEFINES were you passing to gyp_skia to test this?
> 
> I figured it out.  I'm using: 
> 
>   skia_giflib_static=1
>   skia_libpng_static=1
>   skia_zlib_static=1
>   skia_freetype_static=1
>   skia_no_fontconfig=1
>   skia_poppler_enabled=0
>   skia_skip_gui=1
>   skia_gpu=0
>   skia_arch_type=arm64
>   skia_arch_width=64
>   arm_thumb=1
> 
> for testing your patch.

arm_thumb doesn't make sense for AArch64. If for whatever reason you need it,
that means we have something to fix somewhere. Could you try without it and let
me know if something fails?

[hal.canary, 2014-03-20 13:55:27]

https://codereview.chromium.org/143423004/diff/30001/gyp/common_conditions.gypi
File gyp/common_conditions.gypi (right):

https://codereview.chromium.org/143423004/diff/30001/gyp/common_conditions.gy...
gyp/common_conditions.gypi:13: '-ffp-contract=off',
Why are you adding this?  We don't use this for any other arch.

[kevin.petit, 2014-03-20 14:03:43]

On 2014/03/20 13:55:27, Hal Canary wrote:
>
https://codereview.chromium.org/143423004/diff/30001/gyp/common_conditions.gypi
> File gyp/common_conditions.gypi (right):
> 
>
https://codereview.chromium.org/143423004/diff/30001/gyp/common_conditions.gy...
> gyp/common_conditions.gypi:13: '-ffp-contract=off',
> Why are you adding this?  We don't use this for any other arch.

I've made a note in the commit message. Basically multiply-accumulate is fused
by default on AArch64 and it made one of the tests fail. Maybe tolerances can be
made higher on the failing test but I didn't know if this could have
consequences in other places.

[mtklein, 2014-03-20 14:15:25]

On 2014/03/20 14:03:43, kevin.petit wrote:
> On 2014/03/20 13:55:27, Hal Canary wrote:
> >
>
https://codereview.chromium.org/143423004/diff/30001/gyp/common_conditions.gypi
> > File gyp/common_conditions.gypi (right):
> > 
> >
>
https://codereview.chromium.org/143423004/diff/30001/gyp/common_conditions.gy...
> > gyp/common_conditions.gypi:13: '-ffp-contract=off',
> > Why are you adding this?  We don't use this for any other arch.
> 
> I've made a note in the commit message. Basically multiply-accumulate is fused
> by default on AArch64 and it made one of the tests fail. Maybe tolerances can
be
> made higher on the failing test but I didn't know if this could have
> consequences in other places.

I'd be quite happy if the tests read like this if it meant we could keep FMA on:

// Processors with fused multiply-add instructions will generally have higher
precision results here than ones without.
#ifdef SK_HAS_FMA
  const double tolerance = ...;
#else
  const double tolerance = ...;
#endif

[kevin.petit, 2014-03-20 14:21:04]

On 2014/03/20 14:15:25, mtklein wrote:
> On 2014/03/20 14:03:43, kevin.petit wrote:
> > On 2014/03/20 13:55:27, Hal Canary wrote:
> > >
> >
>
https://codereview.chromium.org/143423004/diff/30001/gyp/common_conditions.gypi
> > > File gyp/common_conditions.gypi (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/143423004/diff/30001/gyp/common_conditions.gy...
> > > gyp/common_conditions.gypi:13: '-ffp-contract=off',
> > > Why are you adding this?  We don't use this for any other arch.
> > 
> > I've made a note in the commit message. Basically multiply-accumulate is
fused
> > by default on AArch64 and it made one of the tests fail. Maybe tolerances
can
> be
> > made higher on the failing test but I didn't know if this could have
> > consequences in other places.
> 
> I'd be quite happy if the tests read like this if it meant we could keep FMA
on:
> 
> // Processors with fused multiply-add instructions will generally have higher
> precision results here than ones without.
> #ifdef SK_HAS_FMA
>   const double tolerance = ...;
> #else
>   const double tolerance = ...;
> #endif

From what I've seen it's a bit more tricky. FMA leads to a better precision but
the failing test was comparing results which could benefit from FMA with others
which couldn't thus leading to an unexpectedly big gap. If pieces of core code
rely on the fact that some values computed via different code paths have to be
within a certain range, then it could lead to issues. Like I already said, I'm
not intimate enough with the inner workings of Skia to say for sure if this is a
real problem.

[mtklein, 2014-03-20 14:23:44]

On 2014/03/20 14:21:04, kevin.petit wrote:
> On 2014/03/20 14:15:25, mtklein wrote:
> > On 2014/03/20 14:03:43, kevin.petit wrote:
> > > On 2014/03/20 13:55:27, Hal Canary wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/143423004/diff/30001/gyp/common_conditions.gypi
> > > > File gyp/common_conditions.gypi (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/143423004/diff/30001/gyp/common_conditions.gy...
> > > > gyp/common_conditions.gypi:13: '-ffp-contract=off',
> > > > Why are you adding this?  We don't use this for any other arch.
> > > 
> > > I've made a note in the commit message. Basically multiply-accumulate is
> fused
> > > by default on AArch64 and it made one of the tests fail. Maybe tolerances
> can
> > be
> > > made higher on the failing test but I didn't know if this could have
> > > consequences in other places.
> > 
> > I'd be quite happy if the tests read like this if it meant we could keep FMA
> on:
> > 
> > // Processors with fused multiply-add instructions will generally have
higher
> > precision results here than ones without.
> > #ifdef SK_HAS_FMA
> >   const double tolerance = ...;
> > #else
> >   const double tolerance = ...;
> > #endif
> 
> From what I've seen it's a bit more tricky. FMA leads to a better precision
but
> the failing test was comparing results which could benefit from FMA with
others
> which couldn't thus leading to an unexpectedly big gap. If pieces of core code
> rely on the fact that some values computed via different code paths have to be
> within a certain range, then it could lead to issues. Like I already said, I'm
> not intimate enough with the inner workings of Skia to say for sure if this is
a
> real problem.

Let's try allowing FMA in a followup patch and see what breaks.  We'll just have
to patch tests/ until things work, then we can take a look at the bot GM renders
to get a broad look at how much it affects.  If you do this next week you will
have the advantage of a very cooperative tree sheriff, me.

[kevin.petit, 2014-03-20 14:32:14]

On 2014/03/20 14:23:44, mtklein wrote:
> On 2014/03/20 14:21:04, kevin.petit wrote:
> > On 2014/03/20 14:15:25, mtklein wrote:
> > > On 2014/03/20 14:03:43, kevin.petit wrote:
> > > > On 2014/03/20 13:55:27, Hal Canary wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/143423004/diff/30001/gyp/common_conditions.gypi
> > > > > File gyp/common_conditions.gypi (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/143423004/diff/30001/gyp/common_conditions.gy...
> > > > > gyp/common_conditions.gypi:13: '-ffp-contract=off',
> > > > > Why are you adding this?  We don't use this for any other arch.
> > > > 
> > > > I've made a note in the commit message. Basically multiply-accumulate is
> > fused
> > > > by default on AArch64 and it made one of the tests fail. Maybe
tolerances
> > can
> > > be
> > > > made higher on the failing test but I didn't know if this could have
> > > > consequences in other places.
> > > 
> > > I'd be quite happy if the tests read like this if it meant we could keep
FMA
> > on:
> > > 
> > > // Processors with fused multiply-add instructions will generally have
> higher
> > > precision results here than ones without.
> > > #ifdef SK_HAS_FMA
> > >   const double tolerance = ...;
> > > #else
> > >   const double tolerance = ...;
> > > #endif
> > 
> > From what I've seen it's a bit more tricky. FMA leads to a better precision
> but
> > the failing test was comparing results which could benefit from FMA with
> others
> > which couldn't thus leading to an unexpectedly big gap. If pieces of core
code
> > rely on the fact that some values computed via different code paths have to
be
> > within a certain range, then it could lead to issues. Like I already said,
I'm
> > not intimate enough with the inner workings of Skia to say for sure if this
is
> a
> > real problem.
> 
> Let's try allowing FMA in a followup patch and see what breaks.  We'll just
have
> to patch tests/ until things work, then we can take a look at the bot GM
renders
> to get a broad look at how much it affects.  If you do this next week you will
> have the advantage of a very cooperative tree sheriff, me.

Ok, thanks. Reading between the lines, I understand that you guys are planning
to land this patch before next week, aren't you?

[djsollen, 2014-03-20 14:40:15]

https://codereview.chromium.org/143423004/diff/30001/gyp/common_conditions.gypi
File gyp/common_conditions.gypi (right):

https://codereview.chromium.org/143423004/diff/30001/gyp/common_conditions.gy...
gyp/common_conditions.gypi:13: '-ffp-contract=off',
On 2014/03/20 13:55:27, Hal Canary wrote:
> Why are you adding this?  We don't use this for any other arch.

What test fails? Does the test only fail in release or debug mode? or both?  We
want to go the fast route and we tolerate these differences on x86 between 32
and 64-bit build so we should have tolerance here as well.

https://codereview.chromium.org/143423004/diff/30001/gyp/opts.gyp
File gyp/opts.gyp (right):

https://codereview.chromium.org/143423004/diff/30001/gyp/opts.gyp#newcode117
gyp/opts.gyp:117: [ 'skia_arch_type == "arm64"', {
I'm inclined to stick with the way we handle arch_type and arch_width for x86. 
I know that would mean overhauling this gyp file to be explicit about width 32
vs 64, but think it may be worth it.

Don't make the change yet as I'm going to check with the chrome team on their
approach.  Further, even if we want to make that change I think I could be
convinced to check it in this way first and do a follow on CL.

https://codereview.chromium.org/143423004/diff/30001/src/core/SkBlitter_RGB16...
File src/core/SkBlitter_RGB16.cpp (right):

https://codereview.chromium.org/143423004/diff/30001/src/core/SkBlitter_RGB16...
src/core/SkBlitter_RGB16.cpp:19: #if SK_ARM_NEON_IS_ALWAYS &&
defined(SK_CPU_LENDIAN)
can we do this in a separate CL since it doesn't seem to be tied to 64-bit? If
you want I'll be happy to take care of that.

https://codereview.chromium.org/143423004/diff/30001/src/opts/SkBitmapProcSta...
File src/opts/SkBitmapProcState_opts_arm.cpp (right):

https://codereview.chromium.org/143423004/diff/30001/src/opts/SkBitmapProcSta...
src/opts/SkBitmapProcState_opts_arm.cpp:18: #if defined(SK_CPU_ARM) &&
SK_ARM_ARCH >= 6 && !defined(SK_CPU_BENDIAN)
does this impl that SK_CPU_ARM is not defined for AARCH64? If you goal is to
just ensure this code does execute in 64-bit mode I think it would be clearer to
say !defined(SK_CPU_ARM64).

https://codereview.chromium.org/143423004/diff/30001/src/opts/SkXfermode_opts...
File src/opts/SkXfermode_opts_arm_neon.cpp (right):

https://codereview.chromium.org/143423004/diff/30001/src/opts/SkXfermode_opts...
src/opts/SkXfermode_opts_arm_neon.cpp:75: cmp16 =
vmovn_high_u32(vmovn_u32(cmp1), cmp2);
do we expect this to be a common ifdef? If so, I think we may want to add a
header that includes a bunch of static inline functions to wrap make these call
sites simpler.

same goes for other places in this file.

[djsollen, 2014-03-20 15:37:27]

https://codereview.chromium.org/143423004/diff/30001/gyp/opts.gyp
File gyp/opts.gyp (right):

https://codereview.chromium.org/143423004/diff/30001/gyp/opts.gyp#newcode117
gyp/opts.gyp:117: [ 'skia_arch_type == "arm64"', {
So chromium is opting for arch_type == arm64 as well. So let's leave this as is
since it will make our gyp files more compatible with theirs.

[kevin.petit, 2014-03-20 16:05:08]

https://codereview.chromium.org/143423004/diff/30001/gyp/common_conditions.gypi
File gyp/common_conditions.gypi (right):

https://codereview.chromium.org/143423004/diff/30001/gyp/common_conditions.gy...
gyp/common_conditions.gypi:13: '-ffp-contract=off',
On 2014/03/20 14:40:16, djsollen wrote:
> On 2014/03/20 13:55:27, Hal Canary wrote:
> > Why are you adding this?  We don't use this for any other arch.
> 
> What test fails? Does the test only fail in release or debug mode? or both? 
We
> want to go the fast route and we tolerate these differences on x86 between 32
> and 64-bit build so we should have tolerance here as well.
> 

It was MatrixTest failing only in release mode.

https://codereview.chromium.org/143423004/diff/30001/gyp/opts.gyp
File gyp/opts.gyp (right):

https://codereview.chromium.org/143423004/diff/30001/gyp/opts.gyp#newcode117
gyp/opts.gyp:117: [ 'skia_arch_type == "arm64"', {
On 2014/03/20 15:37:27, djsollen wrote:
> So chromium is opting for arch_type == arm64 as well. So let's leave this as
is
> since it will make our gyp files more compatible with theirs.

To be honest, this has my preference as AArch64 is a different and incompatible
instruction set.

https://codereview.chromium.org/143423004/diff/30001/src/core/SkBlitter_RGB16...
File src/core/SkBlitter_RGB16.cpp (right):

https://codereview.chromium.org/143423004/diff/30001/src/core/SkBlitter_RGB16...
src/core/SkBlitter_RGB16.cpp:19: #if SK_ARM_NEON_IS_ALWAYS &&
defined(SK_CPU_LENDIAN)
On 2014/03/20 14:40:16, djsollen wrote:
> can we do this in a separate CL since it doesn't seem to be tied to 64-bit? If
> you want I'll be happy to take care of that.

You're right. I've uploaded https://codereview.chromium.org/206543002/.

https://codereview.chromium.org/143423004/diff/30001/src/opts/SkBitmapProcSta...
File src/opts/SkBitmapProcState_opts_arm.cpp (right):

https://codereview.chromium.org/143423004/diff/30001/src/opts/SkBitmapProcSta...
src/opts/SkBitmapProcState_opts_arm.cpp:18: #if defined(SK_CPU_ARM) &&
SK_ARM_ARCH >= 6 && !defined(SK_CPU_BENDIAN)
On 2014/03/20 14:40:16, djsollen wrote:
> does this impl that SK_CPU_ARM is not defined for AARCH64? If you goal is to
> just ensure this code does execute in 64-bit mode I think it would be clearer
to
> say !defined(SK_CPU_ARM64).

Yes, that's the goal. I'll change it in the next patch.

https://codereview.chromium.org/143423004/diff/30001/src/opts/SkBitmapProcSta...
src/opts/SkBitmapProcState_opts_arm.cpp:197: #if defined(SK_CPU_ARM) &&
SK_ARM_ARCH >= 6 && !defined(SK_CPU_BENDIAN)
Note to myself: do the same as above here.

https://codereview.chromium.org/143423004/diff/30001/src/opts/SkXfermode_opts...
File src/opts/SkXfermode_opts_arm_neon.cpp (right):

https://codereview.chromium.org/143423004/diff/30001/src/opts/SkXfermode_opts...
src/opts/SkXfermode_opts_arm_neon.cpp:75: cmp16 =
vmovn_high_u32(vmovn_u32(cmp1), cmp2);
On 2014/03/20 14:40:16, djsollen wrote:
> do we expect this to be a common ifdef? If so, I think we may want to add a
> header that includes a bunch of static inline functions to wrap make these
call
> sites simpler.
> 
> same goes for other places in this file.

This is indeed tempting but I'm not sure I'm willing to do it. I'll think a bit
more about it and come back to you.

[kevin.petit, 2014-03-24 11:42:08]

https://codereview.chromium.org/143423004/diff/150001/include/core/SkOnce.h
File include/core/SkOnce.h (right):

https://codereview.chromium.org/143423004/diff/150001/include/core/SkOnce.h#n...
include/core/SkOnce.h:89: asm volatile("dmb ish" : : : "memory");
This barrier is less restrictive than the one previously used. If I've
understood things correctly, SkOnce is only used for synchronisation between
cores. This one line is not arm64-specific and you might want to see it as a
separate patch.

https://codereview.chromium.org/143423004/diff/150001/src/opts/SkXfermode_opt...
File src/opts/SkXfermode_opts_arm_neon.cpp (right):

https://codereview.chromium.org/143423004/diff/150001/src/opts/SkXfermode_opt...
src/opts/SkXfermode_opts_arm_neon.cpp:50: #endif
I've thought a little more about your suggestion and I'd rather not do it, at
least for now:
- I find it easier to follow the code with direct calls to intrinsics
- The else case ought to be enough in the future

However if you really insist, I'm ready to make a patch.

https://codereview.chromium.org/143423004/diff/150001/src/opts/SkXfermode_opt...
src/opts/SkXfermode_opts_arm_neon.cpp:477: // thanks to sign extension.
This includes a small optimisation which is not arm64-specific. Do you want it
as a separate patch?

[djsollen, 2014-03-24 13:29:07]

https://codereview.chromium.org/143423004/diff/150001/src/opts/SkXfermode_opt...
File src/opts/SkXfermode_opts_arm_neon.cpp (right):

https://codereview.chromium.org/143423004/diff/150001/src/opts/SkXfermode_opt...
src/opts/SkXfermode_opts_arm_neon.cpp:50: #endif
On 2014/03/24 11:42:08, kevin.petit wrote:
> I've thought a little more about your suggestion and I'd rather not do it, at
> least for now:
> - I find it easier to follow the code with direct calls to intrinsics
> - The else case ought to be enough in the future
> 
> However if you really insist, I'm ready to make a patch.

I'm fine to leave it as is for now.  However, if we find ourselves doing this
more in the future in other files I think we have a better case for moving it
into a separate inline function.

https://codereview.chromium.org/143423004/diff/150001/src/opts/SkXfermode_opt...
src/opts/SkXfermode_opts_arm_neon.cpp:477: // thanks to sign extension.
On 2014/03/24 11:42:08, kevin.petit wrote:
> This includes a small optimisation which is not arm64-specific. Do you want it
> as a separate patch?

I think this should be fine here as long as it doesn't change any GMs.

[kevin.petit, 2014-03-24 14:20:24]

https://codereview.chromium.org/143423004/diff/150001/src/opts/SkXfermode_opt...
File src/opts/SkXfermode_opts_arm_neon.cpp (right):

https://codereview.chromium.org/143423004/diff/150001/src/opts/SkXfermode_opt...
src/opts/SkXfermode_opts_arm_neon.cpp:50: #endif
On 2014/03/24 13:29:08, djsollen wrote:
> On 2014/03/24 11:42:08, kevin.petit wrote:
> > I've thought a little more about your suggestion and I'd rather not do it,
at
> > least for now:
> I'm fine to leave it as is for now.  However, if we find ourselves doing this
> more in the future in other files I think we have a better case for moving it
> into a separate inline function.

Agreed.

https://codereview.chromium.org/143423004/diff/150001/src/opts/SkXfermode_opt...
src/opts/SkXfermode_opts_arm_neon.cpp:477: // thanks to sign extension.
On 2014/03/24 13:29:08, djsollen wrote:
> On 2014/03/24 11:42:08, kevin.petit wrote:
> > This includes a small optimisation which is not arm64-specific. Do you want
it
> > as a separate patch?
> 
> I think this should be fine here as long as it doesn't change any GMs.

It doesn't. It just achieves the same thing in a smarter way.

[hal.canary, 2014-03-25 16:41:52]

Kevin,

We now have infrastructure in place to test your patch.  To see what the
Linux/Aarch64 buildbot is doing, go to our buildbot page:

    http://skia-tree-status.appspot.com/buildbots/console

Then click on FYISKia and look at the poorly named "Test / Linux" bot, or jsut
go here:

    http://108.170.219.160:10117/buildslaves/skiabot-linux-vm-001

This bot runs the scripts located in skia//platform_tools/barelinux.

Right now, your CL breaks the trybot, BUT if you modify the 

    platform_tools/barelinux/bin/arm64_make

script to set the gyp defines correctly, the tryjob should work well.

[kevin.petit, 2014-03-25 17:02:39]

On 2014/03/25 16:41:52, Hal Canary wrote:
> Kevin,
> 
> We now have infrastructure in place to test your patch.  To see what the
> Linux/Aarch64 buildbot is doing, go to our buildbot page:
> 
>     http://skia-tree-status.appspot.com/buildbots/console
> 
> Then click on FYISKia and look at the poorly named "Test / Linux" bot, or jsut
> go here:
> 
>     http://108.170.219.160:10117/buildslaves/skiabot-linux-vm-001
> 
> This bot runs the scripts located in skia//platform_tools/barelinux.
> 
> Right now, your CL breaks the trybot, BUT if you modify the 
> 
>     platform_tools/barelinux/bin/arm64_make
> 
> script to set the gyp defines correctly, the tryjob should work well.

Great news! Do you mean I should modify the script as part of this patch?

[hal.canary, 2014-03-25 17:05:25]

On 2014/03/25 17:02:39, kevin.petit wrote:
> Great news! Do you mean I should modify the script as part of this patch?

Exactly!

[djsollen, 2014-03-25 17:36:45]

On 2014/03/25 17:05:25, Hal Canary wrote:
> On 2014/03/25 17:02:39, kevin.petit wrote:
> > Great news! Do you mean I should modify the script as part of this patch?
> 
> Exactly!

Hal, do you have a recommended set of GYP defines that he should use?

[hal.canary, 2014-03-25 17:52:16]

On 2014/03/25 17:36:45, djsollen wrote:
> On 2014/03/25 17:05:25, Hal Canary wrote:
> > On 2014/03/25 17:02:39, kevin.petit wrote:
> > > Great news! Do you mean I should modify the script as part of this patch?
> > 
> > Exactly!
> 
> Hal, do you have a recommended set of GYP defines that he should use?

It should be as easy as replacing all the defines in arm64_make with
"skia_arch_type=arm64 linux_use_tcmalloc=0 skia_gpu=0".

[kevin.petit, 2014-03-25 17:54:09]

On 2014/03/25 17:52:16, Hal Canary wrote:
> On 2014/03/25 17:36:45, djsollen wrote:
> > On 2014/03/25 17:05:25, Hal Canary wrote:
> > > On 2014/03/25 17:02:39, kevin.petit wrote:
> > > > Great news! Do you mean I should modify the script as part of this
patch?
> > > 
> > > Exactly!
> > 
> > Hal, do you have a recommended set of GYP defines that he should use?
> 
> It should be as easy as replacing all the defines in arm64_make with
> "skia_arch_type=arm64 linux_use_tcmalloc=0 skia_gpu=0".

I've already uploaded a new patch but I haven't added linux_tcmalloc=0 (I'm not
sure this is needed any more).

[hal.canary, 2014-03-25 18:02:45]

Can you add this to common_variables.gypi:

        [ 'skia_arch_type == "arm64"',
          {
            'skia_arch_width%': 64
          }, {
            [ 'skia_os in ["linux", "freebsd", "openbsd", "solaris", "mac"]', {
              'skia_arch_width%': 64,
            }, {
              'skia_arch_width%': 32,
            }],
          }
        ],

Then you can leave skia_arch_width out of arm64_make.

[kevin.petit, 2014-03-25 18:21:28]

On 2014/03/25 18:02:45, Hal Canary wrote:
> Can you add this to common_variables.gypi:
> 
>         [ 'skia_arch_type == "arm64"',
>           {
>             'skia_arch_width%': 64
>           }, {
>             [ 'skia_os in ["linux", "freebsd", "openbsd", "solaris", "mac"]',
{
>               'skia_arch_width%': 64,
>             }, {
>               'skia_arch_width%': 32,
>             }],
>           }
>         ],
> 
> Then you can leave skia_arch_width out of arm64_make.

Good idea, done. Apparently you have to add a 'conditions' in the else case...
but I'm no gyp expert.

[hal.canary, 2014-03-25 18:24:54]

On 2014/03/25 18:21:28, kevin.petit wrote:
> Good idea, done. Apparently you have to add a 'conditions' in the else case...
> but I'm no gyp expert.


Right.  My mistake.

[djsollen, 2014-03-28 14:31:24]

looks good with one minor change.  As soon as we make that I'm happy to land
this patch.

https://codereview.chromium.org/143423004/diff/270001/gyp/common_variables.gypi
File gyp/common_variables.gypi (left):

https://codereview.chromium.org/143423004/diff/270001/gyp/common_variables.gy...
gyp/common_variables.gypi:75: [ 'skia_os in ["linux", "freebsd", "openbsd",
"solaris", "mac"]', {
just add 'or skia_arch_type == "arm64"' to this condition and remove that
embedded condition.

[hal.canary, 2014-03-28 14:31:43]

looks ready to commit to me.

[kevin.petit, 2014-03-28 14:42:49]

On 2014/03/28 14:31:24, djsollen wrote:
> looks good with one minor change.  As soon as we make that I'm happy to land
> this patch.
> 
>
https://codereview.chromium.org/143423004/diff/270001/gyp/common_variables.gypi
> File gyp/common_variables.gypi (left):
> 
>
https://codereview.chromium.org/143423004/diff/270001/gyp/common_variables.gy...
> gyp/common_variables.gypi:75: [ 'skia_os in ["linux", "freebsd", "openbsd",
> "solaris", "mac"]', {
> just add 'or skia_arch_type == "arm64"' to this condition and remove that
> embedded condition.

Yes, much better. I've uploaded a new patch.

[djsollen, 2014-03-28 15:03:04]

lgtm

[kevin.petit, 2014-03-28 16:19:09]

The CQ bit was checked by kevin.petit@arm.com

[commit-bot: I haz the power, 2014-03-28 16:19:18]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/kevin.petit@arm.com/143423004/290001

[commit-bot: I haz the power, 2014-03-28 16:19:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-28 16:19:24]

Presubmit check for 143423004-290001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Since the CL is editing public API, you must have an LGTM from one of:
('reed@chromium.org', 'reed@google.com', 'bsalomon@chromium.org',
'bsalomon@google.com')

Presubmit checks took 1.1s to calculate.

[kevin.petit, 2014-03-28 16:21:23]

On 2014/03/28 16:19:24, I haz the power (commit-bot) wrote:
> Presubmit check for 143423004-290001 failed and returned exit status 1.
> 
> Running presubmit commit checks ...
> 
> ** Presubmit ERRORS **
> Since the CL is editing public API, you must have an LGTM from one of:
> (mailto:, mailto:, mailto:,
> mailto:)
> 
> Presubmit checks took 1.1s to calculate.

I don't quite understand this message. What is considered public API in this
patch?

[reed1, 2014-03-28 16:28:38]

public api lgtm

[kevin.petit, 2014-03-28 16:32:17]

The CQ bit was checked by kevin.petit@arm.com

[commit-bot: I haz the power, 2014-03-28 16:32:27]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/kevin.petit@arm.com/143423004/290001

[commit-bot: I haz the power, 2014-03-28 17:56:17]

Change committed as 13980

[mtklein, 2014-03-28 18:05:13]

A revert of this CL has been created in
https://codereview.chromium.org/216113005/ by mtklein@google.com.

The reason for reverting is: GYP's failing on most (all?) bots..

[djsollen, 2014-03-28 20:53:22]

https://codereview.chromium.org/143423004/diff/290001/gyp/common_variables.gypi
File gyp/common_variables.gypi (right):

https://codereview.chromium.org/143423004/diff/290001/gyp/common_variables.gy...
gyp/common_variables.gypi:75: [ 'skia_os in ["linux", "freebsd", "openbsd",
"solaris", "mac"] or skia_arch_type == "arm64"', {
this needs to move below line 93 so that skia_arch_type is defined for all
platforms when we do this check.

[kevin.petit, 2014-04-01 10:30:31]

https://codereview.chromium.org/143423004/diff/290001/gyp/common_variables.gypi
File gyp/common_variables.gypi (right):

https://codereview.chromium.org/143423004/diff/290001/gyp/common_variables.gy...
gyp/common_variables.gypi:75: [ 'skia_os in ["linux", "freebsd", "openbsd",
"solaris", "mac"] or skia_arch_type == "arm64"', {
On 2014/03/28 20:53:22, djsollen wrote:
> this needs to move below line 93 so that skia_arch_type is defined for all
> platforms when we do this check.

Done.

[kevin.petit, 2014-04-01 16:23:08]

I've had a look at the trybot results. As far as I can say, the first failure
was related to the Xfermode SSE optimisation which has since been reverted.
However, I find the second one (gyp failing with VS2010/Win7) quite puzzling:
how can skia_arch_type not be defined at this point given the lines above?

[djsollen, 2014-04-01 18:52:17]

sorry for the run around on this.  If this doesn't work then I'll be happy to
take over the patch and land it myself after working out any remaining bugs.

https://codereview.chromium.org/143423004/diff/300001/gyp/common_variables.gypi
File gyp/common_variables.gypi (right):

https://codereview.chromium.org/143423004/diff/300001/gyp/common_variables.gy...
gyp/common_variables.gypi:49: 'skia_android_framework%': 0,
so I was wrong about you needing to move it.  It looks like python doesn't
guarantee that it executes the conditions in order so you need to define
skia_arch_type here like this...

'skia_arch_type%' : 'x86',

then on line 56 put this...

'skia_arch_type%': '<(skia_arch_type)',

https://codereview.chromium.org/143423004/diff/300001/gyp/common_variables.gy...
gyp/common_variables.gypi:89: [ 'skia_os in ["linux", "freebsd", "openbsd",
"solaris", "mac"] or skia_arch_type == "arm64"', {
you can move this back to its original spot.

[kevin.petit, 2014-04-02 13:16:47]

No worries, this should hopefully be the last round.

https://codereview.chromium.org/143423004/diff/300001/gyp/common_variables.gypi
File gyp/common_variables.gypi (right):

https://codereview.chromium.org/143423004/diff/300001/gyp/common_variables.gy...
gyp/common_variables.gypi:49: 'skia_android_framework%': 0,
On 2014/04/01 18:52:17, djsollen wrote:
> so I was wrong about you needing to move it.  It looks like python doesn't
> guarantee that it executes the conditions in order so you need to define
> skia_arch_type here like this...
> 
> 'skia_arch_type%' : 'x86',
> 
> then on line 56 put this...
> 
> 'skia_arch_type%': '<(skia_arch_type)', 

Done.

https://codereview.chromium.org/143423004/diff/300001/gyp/common_variables.gy...
gyp/common_variables.gypi:89: [ 'skia_os in ["linux", "freebsd", "openbsd",
"solaris", "mac"] or skia_arch_type == "arm64"', {
On 2014/04/01 18:52:17, djsollen wrote:
> you can move this back to its original spot.

Done.

[kevin.petit, 2014-04-02 15:03:29]

The CQ bit was checked by kevin.petit@arm.com

[commit-bot: I haz the power, 2014-04-02 15:03:42]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/kevin.petit@arm.com/143423004/320001

[commit-bot: I haz the power, 2014-04-02 15:04:02]

Change committed as 14025