ARM Skia NEON patches - 30 - Xfermode: NEON modeprocs

ARM Skia NEON patches - 30 - Xfermode: NEON modeprocs




Xfermode: NEON implementation of SIMD procs

This patch contains a NEON implementation for a number of Xfermodes.
It provides a big speedup on Xfermode benchmarks (currently up to 3x
with gcc4.7 but up to 10x when gcc produces optimal code for it).

Signed-off-by: Kévin PETIT <kevin.petit@arm.com>;

BUG=

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

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

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

[kevin.petit.not.used.account, 2013-10-09 16:04:08]

This is the NEON code that make https://codereview.chromium.org/23644006/
useful.

[djsollen, 2013-10-09 16:16:57]

doesn't this code need to be compiled with '-mfpu=neon'? I believe that the
current way that this is structured won't work when we run with neon on
conditionally.

[kevin.petit.not.used.account, 2013-10-09 17:23:08]

On 2013/10/09 16:16:57, djsollen wrote:
> doesn't this code need to be compiled with '-mfpu=neon'?

Yes it does.

> I believe that the
> current way that this is structured won't work when we run with neon on
> conditionally.

You're right. I got bitten by an environment variable. It can't work with that
structure.

[kevin.petit.not.used.account, 2013-10-10 11:27:21]

Patch Set 2 works in all cases (no NEON / optional NEON / always NEON) and
allows to keep the diff understandable.

An important thing to note, I think, is that none of the buildbots are building
with optional NEON. This mistake should have been caught on the previous review.

[djsollen, 2013-10-10 12:47:46]

Your right that we should have a bot that builds with NEON conditionally.  I'll
follow up with our infrastructure team about this.

https://codereview.chromium.org/26627004/diff/6001/gyp/opts.gyp
File gyp/opts.gyp (right):

https://codereview.chromium.org/26627004/diff/6001/gyp/opts.gyp#newcode176
gyp/opts.gyp:176: '../src/opts/SkXfermode_opts_arm.cpp',
rename to *opts_arm_neon.cpp

https://codereview.chromium.org/26627004/diff/6001/src/opts/SkXfermode_opts_n...
File src/opts/SkXfermode_opts_none.cpp (right):

https://codereview.chromium.org/26627004/diff/6001/src/opts/SkXfermode_opts_n...
src/opts/SkXfermode_opts_none.cpp:5: #if SK_ARM_NEON_IS_NONE
I would create an *_opts_arm.cpp that has this change and leave *_none.cpp as
is.  I know it seems a bit redundant but it keeps the separation cleaner and in
the event that we add optimizations for other platforms we don't end up with a
bunch of ifdefs in this file.

[kevin.petit.not.used.account, 2013-10-10 13:18:26]

I agree with your comments and that was exactly my first idea but then I
realized the diff would show everything moving. I thought it wasn't the best
thing to do to keep the change readable. Having said that I see three options:

1) Leave as is and fix later with a commit that does only that
2) Move everything like you said if you're not bothered by readability
3) Make another separate commit that prepares the ground for this one

Choose what you prefer and I'll do it.

[djsollen, 2013-10-10 13:20:39]

Option 2 or 3 work for me.  For #2 it may take a little longer, but I'm sure I
can parse through it.

[kevin.petit.not.used.account, 2013-10-10 14:02:25]

Option 2 it is then.

What you want to review is the code above (inclusive) multiply_modeproc_neon8
and the minor modifications to the factory at the bottom of
SkXfermode_opts_arm_neon.cpp.

[djsollen, 2013-10-10 14:24:23]

https://codereview.chromium.org/26627004/diff/15001/src/opts/SkXfermode_opts_...
File src/opts/SkXfermode_opts_arm_neon.cpp (right):

https://codereview.chromium.org/26627004/diff/15001/src/opts/SkXfermode_opts_...
src/opts/SkXfermode_opts_arm_neon.cpp:696: if ((sk_cpu_arm_has_neon()) &&
(gNEONXfermodeProcs[mode] != NULL)) {
my concern with putting this here is that this file is built with NEON. So there
is nothing from preventing the compiler from putting neon instructions in this
function.  However if NEON_IS_DYNAMIC and the device does not have neon then we
have problems.  I think the only way to get around this is to extern a function
from this file and use SK_ARM_NEON_WRAP in the opts_arm.cpp to select the right
function.

You can look at SkBlitRow_opts_* for reference.

[kevin.petit.not.used.account, 2013-10-10 14:40:14]

On 2013/10/10 14:24:23, djsollen wrote:
>
https://codereview.chromium.org/26627004/diff/15001/src/opts/SkXfermode_opts_...
> File src/opts/SkXfermode_opts_arm_neon.cpp (right):
> 
>
https://codereview.chromium.org/26627004/diff/15001/src/opts/SkXfermode_opts_...
> src/opts/SkXfermode_opts_arm_neon.cpp:696: if ((sk_cpu_arm_has_neon()) &&
> (gNEONXfermodeProcs[mode] != NULL)) {
> my concern with putting this here is that this file is built with NEON. So
there
> is nothing from preventing the compiler from putting neon instructions in this
> function.  However if NEON_IS_DYNAMIC and the device does not have neon then
we
> have problems.  I think the only way to get around this is to extern a
function
> from this file and use SK_ARM_NEON_WRAP in the opts_arm.cpp to select the
right
> function.
> 
> You can look at SkBlitRow_opts_* for reference.

It's highly unlikely but still possible. I've fixed it with your suggestion.

[djsollen, 2013-10-10 16:19:28]

looks like the serialization code isn't complete in that it doesn't support
serialization.

[kevin.petit.not.used.account, 2013-10-10 16:26:21]

On 2013/10/10 16:19:28, djsollen wrote:
> looks like the serialization code isn't complete in that it doesn't support
> serialization.

Yes, that looks like a good summary :-). I'm trying to understand why.

[djsollen, 2013-10-10 16:33:19]

If we are serializing to a file that automatically puts us into cross process
mode which your flatten function doesn't handle.  It seems like in that case we
just want to serialize the type of XferMode that we are so that we can fill in
our proc on the other side when we are deserialized.

[kevin.petit.not.used.account, 2013-10-10 16:37:22]

On 2013/10/10 16:33:19, djsollen wrote:
> If we are serializing to a file that automatically puts us into cross process
> mode which your flatten function doesn't handle.  It seems like in that case
we
> just want to serialize the type of XferMode that we are so that we can fill in
> our proc on the other side when we are deserialized.

Thanks for the tip. I think I need an equivalent of setProc(). What I don't
understand however is why it doesn't crash.

[kevin.petit.not.used.account, 2013-10-10 17:32:22]

After a little more investigations, and unless I've missed something really
obvious, it seems to be trickier what I thought I had understood.

My test is "./out/Release/gm -r ../skref/ --config 8888 --resourcePath
resources/ --serialize -w ./gmout/ --match xfermodes3".

I've tried to remove my flatten function altogether and reconstruct the object
from the mode. The traces I've put confirm that the reconstructed object
contains a correct fProcSIMD. However the xfer32 function is never called after
the object has been deserialized and thus the SIMD proc never used. I'm starting
to think that I don't understand at all what this test is about. May I ask you a
few more background information?

[kevin.petit.not.used.account, 2013-10-11 10:39:49]

I think I've found the issue. There's no entry for SkNEONProcCoeffXfermode in
the registrar group at the bottom of SkXfermode.cpp.

If this is the problem, that means the definition of SkNEONProcCoeffXfermode has
to be visible from SkXfermode.cpp, which means pulling arm_neon.h and having to
build with -mfpu=neon: the very thing we've been trying to avoid.

An ugly solution would be to put the declaration of SkNEONProcCoeffXfermode in a
separate header included in SkXfermode.cpp, declare its fProcSIMD member as
void* and cast it to the correct type before use.

What do you think?

[kevin.petit.not.used.account, 2013-10-11 11:52:29]

I've uploaded a patch with my ugly idea. Everything seems to be working this
time.

[djsollen, 2013-10-11 17:12:23]

On 2013/10/11 11:52:29, kevin.petit.arm wrote:
> I've uploaded a patch with my ugly idea. Everything seems to be working this
> time.

I'm out of the office today, so I'm not in a position to pull this down and see
if I can figure out a better way.  This may be the way we have to go given how
the registration code is written, but before I concede to that I want to try
some other approaches.

[kevin.petit.not.used.account, 2013-10-14 13:09:08]

On 2013/10/11 17:12:23, djsollen wrote:
> I'm out of the office today, so I'm not in a position to pull this down and
see
> if I can figure out a better way.  This may be the way we have to go given how
> the registration code is written, but before I concede to that I want to try
> some other approaches.

Thanks for letting me know. I'd happily take suggestions.

[djsollen, 2013-10-14 14:12:24]

If we need to do this more in the future I think I would like to refactor it so
that each platform implements a function that registers any platform specific
flattenables. That seems overkill for just one client so I'm fine with what
you've implemented.

I've just got one nit and then we should run it through a GM test pass before we
submit.

https://codereview.chromium.org/26627004/diff/31001/src/opts/SkXfermode_opts_...
File src/opts/SkXfermode_opts_arm_neon.cpp (right):

https://codereview.chromium.org/26627004/diff/31001/src/opts/SkXfermode_opts_...
src/opts/SkXfermode_opts_arm_neon.cpp:577: 
if (NULL == aa) {
  this->INHERITED::xfer32(...);
  return;
}

If you put this here you can remove your else clause.

[kevin.petit.not.used.account, 2013-10-14 14:15:56]

https://codereview.chromium.org/26627004/diff/31001/src/opts/SkXfermode_opts_...
File src/opts/SkXfermode_opts_arm_neon.cpp (right):

https://codereview.chromium.org/26627004/diff/31001/src/opts/SkXfermode_opts_...
src/opts/SkXfermode_opts_arm_neon.cpp:577: 
On 2013/10/14 14:12:25, djsollen wrote:
> if (NULL == aa) {
>   this->INHERITED::xfer32(...);
>   return;
> }
> 
> If you put this here you can remove your else clause.

That's a good idea but further patches I have need the else clause to be
present.

[kevin.petit.not.used.account, 2013-10-14 14:18:12]

On 2013/10/14 14:12:24, djsollen wrote:
> If we need to do this more in the future I think I would like to refactor it
so
> that each platform implements a function that registers any platform specific
> flattenables. That seems overkill for just one client so I'm fine with what
> you've implemented.

That would be much cleaner. If someone implements a SSE version of this, he'll
have this as a bonus task :-).

[djsollen, 2013-10-14 15:50:09]

So I'm good with the structure of the code, but the functions themselves have
some problems.  If you run gm both before and after your changes with "--match
xfermode --config 8888 -w /sdcard/skia_xfer_results" and copy the results to
your machine you'll find that many of the outputs do not match.

[kevin.petit.not.used.account, 2013-10-14 16:18:41]

On 2013/10/14 15:50:09, djsollen wrote:
> So I'm good with the structure of the code, but the functions themselves have
> some problems.  If you run gm both before and after your changes with "--match
> xfermode --config 8888 -w /sdcard/skia_xfer_results" and copy the results to
> your machine you'll find that many of the outputs do not match.

I can't reproduce the problem. I've built the tree at r11654, generated the GMs
with "-w ../skref/ --config 8888  --resourcePath resources/". I then build with
this patch applied and run gm with "-r ../skref/ --config 8888  --resourcePath
resources/". No mismatches, which is what I expect. I'll have a closer look at
the trybot results to see if I can spot anything.

[djsollen, 2013-10-14 16:28:22]

The differences on my side are pretty glaring.  Have you tried writing both of
them out and looking at their results visually?

[kevin.petit.not.used.account, 2013-10-14 16:28:38]

Are the reference images against which the output is compared the very latest
ones or the one corresponding to the base for a change? Do you think rebasing
the patch and resending could make a difference?

[djsollen, 2013-10-14 16:32:03]

On 2013/10/14 16:28:38, kevin.petit.arm wrote:
> Are the reference images against which the output is compared the very latest
> ones or the one corresponding to the base for a change? Do you think rebasing
> the patch and resending could make a difference?

I don't think so.  I pulled down your latest patch set (#6) and ran it locally
both with and without that patch.  I then took the results and ran it through
skdiff to visualize the results.  I also just ran it using the -r option on the
second pass and I'm getting failures there as well.

[kevin.petit.not.used.account, 2013-10-14 17:30:33]

I've checked that Patch Set 6 was the same code I have.

I've redone everything from scratch (build, generate ref images, rebuild,
compare) on a rebased version of the patch. I still can't get it to fail. I've
tried to introduce a small mistake in the NEON functions and gm fails
immediately. That rules out the "modify something and test something else" kind
of mistakes.

At this point, I don't really know what to say. Do you have some exact
steps/commands you'd like me to test?

[djsollen, 2013-10-14 18:13:49]

What device are you testing on and what are your build commands? I run...

rm -rf out
android_make -d nexus_10 -j gm
android_run_skia gm --config 8888 --match xfer -w /sdcard/xfer_orig
<patch in your change>
rm -rf out
android_make -d nexus_10 -j gm
android_run_skia gm --config 8888 --match xfer -w /sdcard/xfer_neon -r
/sdcard/xfer_orig

OUTPUT:
GM: These configs will be run: 8888
GM: drawing... xfermodes3 [630 1215]
GM: ---- xfermodes3_8888.png: 17985 (of 765450) differing pixels, max
per-channel mismatch R=167 G=191 B=187 A=0
GM: drawing... xfermodes2 [455 475]
GM: drawing... xfermodes [790 640]
GM: ---- xfermodes_8888.png: 45496 (of 505600) differing pixels, max per-channel
mismatch R=153 G=170 B=187 A=0
GM: drawing... xfermodeimagefilter [600 600]
GM: ---- xfermodeimagefilter_8888.png: 23731 (of 360000) differing pixels, max
per-channel mismatch R=131 G=170 B=131 A=0
GM: drawing... mixed_xfermodes [790 640]

[kevin.petit.not.used.account, 2013-10-15 11:57:32]

It was only failing when building in debug mode, that's what caused the
confusion.

Patch Set 7 fixes a (not so) subtle mistake in the constraints of the asm code
in SkNEONProcCoeffXfermode::xfer32().

[djsollen, 2013-10-15 14:26:53]

If the test passes like I believe it will then I think this is good to submit.

[kevin.petit.not.used.account, 2013-10-15 14:44:58]

bsalomon fixed the build issue. Could you retrigger the bot please?

[djsollen, 2013-10-15 15:54:25]

lgtm, I ran the tests locally and its passing so there is no need to wait for
this trybot to complete.

[commit-bot: I haz the power, 2013-10-15 15:54:40]

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

[commit-bot: I haz the power, 2013-10-15 16:18:44]

Change committed as 11777

[robertphillips, 2013-10-16 01:06:35]

This is failing to compile in Chrome:

FAILED: /mnt/scratch0/b_used/build/goma/gomacc
/mnt/scratch0/b_used/build/slave/android_clang_dbg/build/src/third_party/llvm-build/Release+Asserts/bin/clang++
-MMD -MF
obj/third_party/skia/src/opts/skia_opts_neon.SkXfermode_opts_arm_neon.o.d
-DANGLE_DX11 -D_FILE_OFFSET_BITS=64 -DNO_TCMALLOC
-DDISCARDABLE_MEMORY_ALWAYS_SUPPORTED_NATIVELY
-DSYSTEM_NATIVELY_SIGNALS_MEMORY_PRESSURE -DDISABLE_NACL -DCHROMIUM_BUILD
-DCOMPONENT_BUILD -DUSE_LIBJPEG_TURBO=1 -DENABLE_WEBRTC=1
-DUSE_PROPRIETARY_CODECS -DENABLE_CONFIGURATION_POLICY -DUSE_OPENSSL=1
-DENABLE_EGLIMAGE=1 -DENABLE_AUTOFILL_DIALOG=1 -DCLD_VERSION=1
-DENABLE_PRINTING=1 -D__ARM_HAVE_OPTIONAL_NEON_SUPPORT -DANDROID
-D__GNU_SOURCE=1 -DUSE_STLPORT=1 -D_STLP_USE_PTR_SPECIALIZATIONS=1
'-DCHROME_BUILD_ID=""' -DHAVE_SYS_UIO_H -DDYNAMIC_ANNOTATIONS_ENABLED=1
-DWTF_USE_DYNAMIC_ANNOTATIONS=1 -D_DEBUG -I../.. -I../../skia/config
-I../../third_party/skia/include/core -I../../third_party/skia/src/core
-I../../third_party/skia/src/opts -fstack-protector --param=ssp-buffer-size=4
-Werror -fno-exceptions -fno-strict-aliasing -Wno-unused-parameter
-Wno-missing-field-initializers -fvisibility=hidden -pipe -fPIC -Wheader-hygiene
-Wno-char-subscripts -Wno-unused-function -Wno-covered-switch-default
-Wstring-conversion -Wno-c++11-narrowing -Wno-reserved-user-defined-literal
-Wno-deprecated-register -Wno-unused-const-variable -Wno-deprecated-declarations
-Xclang -load -Xclang
/mnt/scratch0/b_used/build/slave/android_clang_dbg/build/src/tools/clang/scripts/../../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so
-Xclang -add-plugin -Xclang find-bad-constructs -fcolor-diagnostics -mfpu=neon
-fomit-frame-pointer -Wno-format -march=armv7-a -mtune=cortex-a8
-mfloat-abi=softfp -mthumb -ffunction-sections -funwind-tables -g
-fstack-protector -fno-short-enums -Wa,--noexecstack
-D__compiler_offsetof=__builtin_offsetof -Dnan=__builtin_nan -target
arm-linux-androideabi -mllvm -arm-enable-ehabi
--sysroot=/mnt/scratch0/b_used/build/slave/android_clang_dbg/build/src/third_party/android_tools/ndk//platforms/android-14/arch-arm
-I/mnt/scratch0/b_used/build/slave/android_clang_dbg/build/src/third_party/android_tools/ndk//sources/cxx-stl/stlport/stlport
-Os -g -fomit-frame-pointer -fdata-sections -ffunction-sections -g0 -fno-rtti
-fno-threadsafe-statics -fvisibility-inlines-hidden -std=gnu++11
-Wno-implicit-exception-spec-mismatch -Wno-deprecated -Wno-abi  -c
../../third_party/skia/src/opts/SkXfermode_opts_arm_neon.cpp -o
obj/third_party/skia/src/opts/skia_opts_neon.SkXfermode_opts_arm_neon.o
Value type is non-standard value, Other.
UNREACHABLE executed at
/usr/local/google/work/chromium/src/third_party/llvm/include/llvm/CodeGen/ValueTypes.h:361!
0  clang     0x0000000002159df5 llvm::sys::PrintStackTrace(_IO_FILE*) + 37
1  clang     0x000000000215a243
2  libc.so.6 0x00007f03e06114a0
3  libc.so.6 0x00007f03e0611425 gsignal + 53
4  libc.so.6 0x00007f03e0614b8b abort + 379
5  clang     0x0000000002148f4f llvm::llvm_unreachable_internal(char const*,
char const*, unsigned int) + 447
6  clang     0x0000000001519e4e
7  clang     0x000000000175f292
llvm::ARMTargetLowering::getRegForInlineAsmConstraint(std::string const&,
llvm::MVT) const + 242
8  clang     0x0000000001a7342b
9  clang     0x0000000001a58c88
llvm::SelectionDAGBuilder::visitInlineAsm(llvm::ImmutableCallSite) + 3512
10 clang     0x0000000001a4c79a
llvm::SelectionDAGBuilder::visitCall(llvm::CallInst const&) + 74
11 clang     0x0000000001a45567
llvm::SelectionDAGBuilder::visit(llvm::Instruction const&) + 71
12 clang     0x0000000001a857e8
llvm::SelectionDAGISel::SelectBasicBlock(llvm::ilist_iterator<llvm::Instruction
const>, llvm::ilist_iterator<llvm::Instruction const>, bool&) + 40
13 clang     0x0000000001a85024
llvm::SelectionDAGISel::SelectAllBasicBlocks(llvm::Function const&) + 4788
14 clang     0x0000000001a8307a
llvm::SelectionDAGISel::runOnMachineFunction(llvm::MachineFunction&) + 1338
15 clang     0x0000000001be293c
llvm::MachineFunctionPass::runOnFunction(llvm::Function&) + 124
16 clang     0x00000000020f5bba
llvm::FPPassManager::runOnFunction(llvm::Function&) + 362
17 clang     0x00000000020f5e4b llvm::FPPassManager::runOnModule(llvm::Module&)
+ 43
18 clang     0x00000000020f61d8 llvm::MPPassManager::runOnModule(llvm::Module&)
+ 440
19 clang     0x00000000020f6b4f llvm::PassManagerImpl::run(llvm::Module&) + 559
20 clang     0x000000000083ef77
clang::EmitBackendOutput(clang::DiagnosticsEngine&, clang::CodeGenOptions
const&, clang::TargetOptions const&, clang::LangOptions const&, llvm::Module*,
clang::BackendAction, llvm::raw_ostream*) + 5959
21 clang     0x000000000083ca16
22 clang     0x00000000006c9c84
clang::MultiplexConsumer::HandleTranslationUnit(clang::ASTContext&) + 68
23 clang     0x00000000009aa463 clang::ParseAST(clang::Sema&, bool, bool) + 515
24 clang     0x000000000083be22 clang::CodeGenAction::ExecuteAction() + 514
25 clang     0x00000000006b2cf0 clang::FrontendAction::Execute() + 112
26 clang     0x000000000069003d
clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) + 925
27 clang     0x00000000006770ba
clang::ExecuteCompilerInvocation(clang::CompilerInstance*) + 2906
28 clang     0x000000000066f3ff cc1_main(char const**, char const**, char
const*, void*) + 687
29 clang     0x000000000067533a main + 8858
30 libc.so.6 0x00007f03e05fc76d __libc_start_main + 237
31 clang     0x000000000066f089
Stack dump:
0.	Program arguments:
/mnt/scratch0/b_used/build/slave/android_clang_dbg/build/src/third_party/llvm-build/Release+Asserts/bin/clang
-cc1 -triple thumbv7--linux-androideabi -S -disable-free -main-file-name
SkXfermode_opts_arm_neon.cpp -mrelocation-model pic -pic-level 2
-relaxed-aliasing -fmath-errno -mconstructor-aliases -munwind-tables
-fuse-init-array -target-cpu cortex-a8 -target-feature +soft-float-abi
-target-feature +neon -target-abi aapcs-linux -mfloat-abi soft
-target-linker-version 2.22 -ffunction-sections -ffunction-sections
-fdata-sections -coverage-file /tmp/SkXfermode_opts_arm_neon-c5ff8a.s
-resource-dir
/mnt/scratch0/b_used/build/slave/android_clang_dbg/build/src/third_party/llvm-build/Release+Asserts/bin/../lib/clang/3.4
-dependency-file
obj/third_party/skia/src/opts/skia_opts_neon.SkXfermode_opts_arm_neon.o.d -MT
obj/third_party/skia/src/opts/skia_opts_neon.SkXfermode_opts_arm_neon.o -D
ANGLE_DX11 -D _FILE_OFFSET_BITS=64 -D NO_TCMALLOC -D
DISCARDABLE_MEMORY_ALWAYS_SUPPORTED_NATIVELY -D
SYSTEM_NATIVELY_SIGNALS_MEMORY_PRESSURE -D DISABLE_NACL -D CHROMIUM_BUILD -D
COMPONENT_BUILD -D USE_LIBJPEG_TURBO=1 -D ENABLE_WEBRTC=1 -D
USE_PROPRIETARY_CODECS -D ENABLE_CONFIGURATION_POLICY -D USE_OPENSSL=1 -D
ENABLE_EGLIMAGE=1 -D ENABLE_AUTOFILL_DIALOG=1 -D CLD_VERSION=1 -D
ENABLE_PRINTING=1 -D __ARM_HAVE_OPTIONAL_NEON_SUPPORT -D ANDROID -D
__GNU_SOURCE=1 -D USE_STLPORT=1 -D _STLP_USE_PTR_SPECIALIZATIONS=1 -D
CHROME_BUILD_ID="" -D HAVE_SYS_UIO_H -D DYNAMIC_ANNOTATIONS_ENABLED=1 -D
WTF_USE_DYNAMIC_ANNOTATIONS=1 -D _DEBUG -D
__compiler_offsetof=__builtin_offsetof -D nan=__builtin_nan -I ../.. -I
../../skia/config -I ../../third_party/skia/include/core -I
../../third_party/skia/src/core -I ../../third_party/skia/src/opts -I
/mnt/scratch0/b_used/build/slave/android_clang_dbg/build/src/third_party/android_tools/ndk//sources/cxx-stl/stlport/stlport
-isysroot
/mnt/scratch0/b_used/build/slave/android_clang_dbg/build/src/third_party/android_tools/ndk//platforms/android-14/arch-arm
-internal-isystem
/mnt/scratch0/b_used/build/slave/android_clang_dbg/build/src/third_party/android_tools/ndk//platforms/android-14/arch-arm/usr/local/include
-internal-isystem
/mnt/scratch0/b_used/build/slave/android_clang_dbg/build/src/third_party/llvm-build/Release+Asserts/bin/../lib/clang/3.4/include
-internal-externc-isystem
/mnt/scratch0/b_used/build/slave/android_clang_dbg/build/src/third_party/android_tools/ndk//platforms/android-14/arch-arm/include
-internal-externc-isystem
/mnt/scratch0/b_used/build/slave/android_clang_dbg/build/src/third_party/android_tools/ndk//platforms/android-14/arch-arm/usr/include
-Os -Werror -Wno-unused-parameter -Wno-missing-field-initializers
-Wheader-hygiene -Wno-char-subscripts -Wno-unused-function
-Wno-covered-switch-default -Wstring-conversion -Wno-c++11-narrowing
-Wno-reserved-user-defined-literal -Wno-deprecated-register
-Wno-unused-const-variable -Wno-deprecated-declarations -Wno-format
-Wno-implicit-exception-spec-mismatch -Wno-deprecated -Wno-abi -std=gnu++11
-fno-dwarf-directory-asm -fdebug-compilation-dir
/mnt/scratch0/b_used/build/slave/android_clang_dbg/build/src/out/Debug
-ferror-limit 19 -fmessage-length 0 -fvisibility hidden
-fvisibility-inlines-hidden -stack-protector 1 -stack-protector-buffer-size 4
-mstackrealign -fno-rtti -fno-signed-char -fno-threadsafe-statics
-fobjc-runtime=gcc -fdiagnostics-show-option -fcolor-diagnostics
-vectorize-loops -vectorize-slp -load
/mnt/scratch0/b_used/build/slave/android_clang_dbg/build/src/tools/clang/scripts/../../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so
-add-plugin find-bad-constructs -mllvm -arm-enable-ehabi -o
/tmp/SkXfermode_opts_arm_neon-c5ff8a.s -x c++
../../third_party/skia/src/opts/SkXfermode_opts_arm_neon.cpp 
1.	<eof> parser at end of file
2.	Code generation
3.	Running pass 'Function Pass Manager' on module
'../../third_party/skia/src/opts/SkXfermode_opts_arm_neon.cpp'.
4.	Running pass 'ARM Instruction Selection' on function
'@_ZNK23SkNEONProcCoeffXfermode6xfer32EPjPKjiPKh'
clang: error: unable to execute command: Aborted
clang: error: clang frontend command failed due to signal (use -v to see
invocation)
clang version 3.4 (trunk 191856)
Target: arm--linux-androideabi
Thread model: posix
clang: note: diagnostic msg: PLEASE submit a bug report to http://llvm.org/bugs/
and include the crash backtrace, preprocessed source, and associated run script.
clang: note: diagnostic msg: 
********************

[robertphillips, 2013-10-16 01:13:48]

reverted in r11799 (please see prior comment for compilation log

[kevin.petit.not.used.account, 2013-10-16 10:44:19]

On 2013/10/16 01:13:48, robertphillips wrote:
> reverted in r11799 (please see prior comment for compilation log

Has this been reported to a toolchain person? Is anybody investigating?

[robertphillips, 2013-10-16 11:41:50]

On 2013/10/16 10:44:19, kevin.petit.arm wrote:
> On 2013/10/16 01:13:48, robertphillips wrote:
> > reverted in r11799 (please see prior comment for compilation log
> 
> Has this been reported to a toolchain person? Is anybody investigating?

Not yet. Do you have any idea what is going on? Is the "Value type is
non-standard value, Other." warning/error significant?

[kevin.petit.not.used.account, 2013-10-16 11:46:59]

On 2013/10/16 11:41:50, robertphillips wrote:
> On 2013/10/16 10:44:19, kevin.petit.arm wrote:
> > On 2013/10/16 01:13:48, robertphillips wrote:
> > > reverted in r11799 (please see prior comment for compilation log
> > 
> > Has this been reported to a toolchain person? Is anybody investigating?
> 
> Not yet. Do you have any idea what is going on? Is the "Value type is
> non-standard value, Other." warning/error significant?

The error is significant, this is displayed by llvm_unreachable() which is a way
to die gracefully on unexpected cases.
I'm building clang/llvm r191856 to see if I can reproduce the issue.

[robertphillips, 2013-10-16 12:10:20]

These logs may be more informative:

android_rel:
http://build.chromium.org/p/tryserver.chromium/builders/android_rel/builds/26...

android_dbg:
http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/11...

android_clang_dbg:
http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...

Since those bits will rot we are getting a lot of errors like:

../../third_party/skia/src/opts/SkXfermode_opts_arm_neon.cpp:626:16: error:
expected ',' before '::' token
../../third_party/skia/src/opts/SkXfermode_opts_arm_neon.cpp:626:16: error:
expected identifier before '::' token
../../third_party/skia/src/opts/SkXfermode_opts_arm_neon.cpp:626:29: error: type
'<lambda>' with no linkage used to declare function 'void<lambda>::operator()()
const' with linkage [-fpermissive]
../../third_party/skia/src/opts/SkXfermode_opts_arm_neon.cpp: In lambda
function:
../../third_party/skia/src/opts/SkXfermode_opts_arm_neon.cpp:626:33: error:
expected '{' before '=' token
../../third_party/skia/src/opts/SkXfermode_opts_arm_neon.cpp: At global scope:
../../third_party/skia/src/opts/SkXfermode_opts_arm_neon.cpp:626:33: warning:
lambda expressions only available with -std=c++11 or -std=gnu++11 [enabled by
default]
../../third_party/skia/src/opts/SkXfermode_opts_arm_neon.cpp:627:16: error:
expected ',' before '::' token
../../third_party/skia/src/opts/SkXfermode_opts_arm_neon.cpp:627:16: error:
expected identifier before '::' token
../../third_party/skia/src/opts/SkXfermode_opts_arm_neon.cpp:627:27: error: type
'<lambda>' with no linkage used to declare function 'void<lambda>::operator()()
const' with linkage [-fpermissive]
../../third_party/skia/src/opts/SkXfermode_opts_arm_neon.cpp: In lambda
function:
../../third_party/skia/src/opts/SkXfermode_opts_arm_neon.cpp:627:33: error:
expected '{' before '=' token
../../third_party/skia/src/opts/SkXfermode_opts_arm_neon.cpp: At global scope:
../../third_party/skia/src/opts/SkXfermode_opts_arm_neon.cpp:627:33: warning:
lambda expressions only available with -std=c++11 or -std=gnu++11 [enabled by
default]
../../third_party/skia/src/opts/SkXfermode_opts_arm_neon.cpp

[kevin.petit.not.used.account, 2013-10-16 12:38:37]

Thanks for the logs.

The problem in the two first logs seems to be the designated initializers I've
used in gNEONXfermodeProcs. Now that I've added a SK_COMPILE_ASSERT, as per
djsollen's suggestion, these designated initializers have become useless. This
is a feature badly supported by compilers (just discovered it's standardized in
C but not C++).

I'll upload a revised version replacing them with a comment. It will fix the two
first builds and, if we're lucky, the clang one too.

[kevin.petit.not.used.account, 2013-10-16 14:11:19]

Could someone reopen the issue so I can submit a fix, please?

[robertphillips, 2013-10-16 14:12:52]

reopened

[kevin.petit.not.used.account, 2013-10-16 14:29:37]

Thanks. However, I don't seem to be able to upload to this issue anymore:

$ git cl upload
Using 50% similarity for rename/copy detection. Override with --similarity.
Can't guess svn branch -- try specifying it on the command line

$ git cl upload --target_branch=master
Using 50% similarity for rename/copy detection. Override with --similarity.
Usage: git cl upload [options] [args to "git diff"]

git cl: error: Use --target_branch for non gerrit repository.

I also tried to remove the branch, create a new one and assign the issue: same
issue.

Any ideas?

[djsollen, 2013-10-16 14:59:40]

have you run git svn rebase lately?

[kevin.petit.not.used.account, 2013-10-16 15:10:40]

On 2013/10/16 14:59:40, djsollen wrote:
> have you run git svn rebase lately?

Nope. I've used my usual combination:

$ git svn fetch

- create a backup branch for the branch corresponding to the issue
- reset the issue branch to the base for my patch

$ git am the_patch_coming_from_the_test_machine.patch
($ git cl status)
$ git cl upload

This has always worked for me.

When I do git cl status, the issue URL for my branch appears in green.

[kevin.petit.not.used.account, 2013-10-16 15:42:14]

I've dug into git_cl.py and understood the problem.

I had done a cherry-pick of the commit done by the bot to tune it and so I had
"git-svn-id" in my commit message but, this is the fun part, with an http url
instead of https.

[commit-bot: I haz the power, 2013-10-16 15:47:49]

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

[commit-bot: I haz the power, 2013-10-16 16:24:15]

Change committed as 11813

[robertphillips, 2013-10-17 00:10:10]

Reverted in r11833 due to Chromium Android build failures. Please see
https://codereview.chromium.org/27304003/.

[kevin.petit.not.used.account, 2013-10-17 12:11:30]

On 2013/10/17 00:10:10, robertphillips wrote:
> Reverted in r11833 due to Chromium Android build failures. Please see
> https://codereview.chromium.org/27304003/.

The problem is building with gcc4.6. It doesn't support the "h" constraint in
the asm (this might be the problem we're observing with clang). I'm trying to
find a way to make this build with 4.6 (having fast code won't be possible) AND
have no performance regressions with later versions which is going to be tricky.

NEON-wise 4.6 is a disaster and should be avoided as much as possible.

Could you reopen the issue, please?

[robertphillips, 2013-10-17 12:19:15]

reopened

[robertphillips, 2013-10-17 12:20:08]

reopened

[kevin.petit.not.used.account, 2013-10-17 14:32:20]

On 2013/10/17 12:20:08, robertphillips wrote:
> reopened

Thank you. Patch Set 9 is not pretty but works with gcc 4.6, 4.7, 4.8 and clang.

Also, I've been able to reproduce the error observed with clang. It is due to
the "h" constraint too. I'll submit a bug report to llvm.

The code generated for 4.6 is far from optimal but I haven't found a workaround
for that. Do you know how long 4.6 will have to be supported?

[commit-bot: I haz the power, 2013-10-17 14:42:09]

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

[commit-bot: I haz the power, 2013-10-17 16:29:38]

Change committed as 11843

[djsollen, 2013-10-17 18:34:17]

https://codereview.chromium.org/26627004/diff/95001/src/opts/SkXfermode_opts_...
File src/opts/SkXfermode_opts_arm_neon.cpp (right):

https://codereview.chromium.org/26627004/diff/95001/src/opts/SkXfermode_opts_...
src/opts/SkXfermode_opts_arm_neon.cpp:583: #if (__GNUC__ == 4) &&
(__GNUC_MINOR__ > 6)
what happens to this code when we go to version 5.x?  Can you please upload a
patch that will handle that?

[kevin.petit.not.used.account, 2013-10-18 15:20:28]

On 2013/10/17 18:34:17, djsollen wrote:
>
https://codereview.chromium.org/26627004/diff/95001/src/opts/SkXfermode_opts_...
> File src/opts/SkXfermode_opts_arm_neon.cpp (right):
> 
>
https://codereview.chromium.org/26627004/diff/95001/src/opts/SkXfermode_opts_...
> src/opts/SkXfermode_opts_arm_neon.cpp:583: #if (__GNUC__ == 4) &&
> (__GNUC_MINOR__ > 6)
> what happens to this code when we go to version 5.x?  Can you please upload a
> patch that will handle that?

It falls back to the else case for which gcc4.8 already generates sensible
(though not optimal) code. To be honest, I hope that before 5.x even exist we
will have gotten rid of all this mess and replaced it with the *two* lines it
should be.

If you don't mind I'd like to wait until Monday (we shouldn't see gcc5.x this
week-end :-)) to see if this stays in the tree for good. My proposal:
- if this gets reverted, it's a no brainer and I'll do something for gcc5.x in
the next patch
- if it stays in the tree I'll submit the change in the review for xfer16 which
will have a similar mechanism

[djsollen, 2013-10-18 18:54:04]

that sounds good to me.

[robertphillips, 2013-10-18 18:55:00]

the DEPS roll seems to have stuck.