Fix simd access for 64bit ARM neon clang compilation

Fix simd access for 64bit ARM neon clang compilation

This patch fixes the invalid operand access of
simd instructions on ARM64

compilation error example :

../../third_party/libjpeg_turbo/simd/jsimd_arm64_neon.S:1859:1:
note: while in macro instantiation
generate_jsimd_ycc_rgb_convert_neon rgb565, 16, 0, .4h, 0, .4h, 0, .4h, .8b
^
<instantiation>:4:31: error: invalid operand for instruction
    smlal v20.4s, v8.4h, v1.4h[2]

This patch helps in building 64bit browser with clang for ARM64

BUG : http://crbug.com/539781

Signed-off-by: Bernhard Rosenkränzer <bero@linaro.org>;

BUG=539781
R=thakis@chromium.org

[Nico, 2016-04-20 20:53:31]

Renato, do you know if all of the changes in this patch are due to the existing
assembly being bad, or is some of it clang not implementing stuff it should
implement?

Generally, we prefer fixing things that are missing upstream upstream instead of
working around them. The .arch thing is something that upstream should accept
for sure.

If the rest is something upstream clang doesn't want to change, please send this
patch to upstream libjpeg_turbo too, and update README.chromium with a link to
the upstream change (example: https://codereview.chromium.org/1810373002/)

https://codereview.chromium.org/1885373002/diff/1/simd/jsimd_arm64_neon.S
File simd/jsimd_arm64_neon.S (right):

https://codereview.chromium.org/1885373002/diff/1/simd/jsimd_arm64_neon.S#new...
simd/jsimd_arm64_neon.S:33: .arch armv8-a+fp+simd
again, can we please fix this in clang instead?

[khasim.mohammed, 2016-04-21 09:43:36]

Hi Nico,

For testing purposes now I took latest
https://github.com/libjpeg-turbo/libjpeg-turbo/blob/master/simd/jsimd_arm64_n...

and replaced with the one in chromium/src/third_party/libjpeg_turbo/simd

It builds fine with clang for 64bit. This doesn't have .arch and has
multiple fixes for assembly.

Any plans to consider latest from libjpeg-turbo project ?

Regards,
Khasim


On 21 April 2016 at 02:23, <thakis@chromium.org> wrote:

> Renato, do you know if all of the changes in this patch are due to the
> existing
> assembly being bad, or is some of it clang not implementing stuff it should
> implement?
>
> Generally, we prefer fixing things that are missing upstream upstream
> instead of
> working around them. The .arch thing is something that upstream should
> accept
> for sure.
>
> If the rest is something upstream clang doesn't want to change, please
> send this
> patch to upstream libjpeg_turbo too, and update README.chromium with a
> link to
> the upstream change (example: https://codereview.chromium.org/1810373002/)
>
>
> https://codereview.chromium.org/1885373002/diff/1/simd/jsimd_arm64_neon.S
> File simd/jsimd_arm64_neon.S (right):
>
>
>
https://codereview.chromium.org/1885373002/diff/1/simd/jsimd_arm64_neon.S#new...
> simd/jsimd_arm64_neon.S:33: .arch armv8-a+fp+simd
> again, can we please fix this in clang instead?
>
> https://codereview.chromium.org/1885373002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[renato.golin_linaro.org, 2016-04-21 17:09:51]

On 20 April 2016 at 21:53,  <thakis@chromium.org> wrote:
> Renato, do you know if all of the changes in this patch are due to the
> existing
> assembly being bad, or is some of it clang not implementing stuff it should
> implement?

Hi Nico,

If I understood correctly, this patch has only two issues:

1. .arch doesn't work.

That's covered by https://llvm.org/bugs/show_bug.cgi?id=26016, and
we're working on the AArch64 TargetParser now. It may take a while
(new engineer), but as Khasim said, this is not present in the new
version, so it might not be needed right now/ever in this file.

2. ins register syntax.

GCC seems to accept the "fully qualified" syntax (ARMv8 ISA 4.4.2.2)
for the INS instruction.

The ARM ARM (C7.3.150) INS section says the syntax is:

INS <Vd>.<Ts>[<index1>], <Vn>.<Ts>[<index2>]

with <Ts> as:

...an element size specifier, encoded in the "imm5" field. It can have
the following values:
B when imm5 = xxxx1
H when imm5 = xxx10
S when imm5 = xx100
D when imm5 = x1000
The encoding imm5 = x0000 is reserved.

No numbers are necessary for the syntax here, since they're just lanes
in the INS point of view (no type size at all), and they're not even
encoded in the instruction itself.

Same is true for other instructions (SMULL) that use the <Ts> field.

So, while this may allow for extra checking (not sure how, though),
it's not strictly necessary. Should be ok to allow that syntax, though
it's an improvement, not a bug.

Feel free to create a bug in LLVM for this, copy me, Tim Northover and
James Molloy.

But this shouldn't stop you, if the new version compiles fine with
Clang, as Khasim mentioned.

cheers,
--renato

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2016-04-21 21:42:23]

Good to hear it works with upstream libjpeg-turbo. Can you try rolling us
forward then?

[khasim.mohammed, 2016-04-22 03:34:56]

Sorry, I didn't get. Do you want me to build with complete libjpeg-turbo
latest sources ?

Regards,
Khasim

On 22 April 2016 at 03:12, <thakis@chromium.org> wrote:

> Good to hear it works with upstream libjpeg-turbo. Can you try rolling us
> forward then?
>
> https://codereview.chromium.org/1885373002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[khasim.mohammed, 2016-04-23 01:57:33]

Nico, can you please suggest on what we should do here, I am assuming these
tasks

a) Migrate to latest libjpeg-turbo, evaluate for ARMv7 and v8 with both GCC
and Clang
b) submit these patches to chromium

Sorry I am new to this community so just trying to ensure I am doing the
right thing.

Regards,
Khasim


On 22 April 2016 at 09:04, Khasim Syed Mohammed <khasim.mohammed@linaro.org>
wrote:

> Sorry, I didn't get. Do you want me to build with complete libjpeg-turbo
> latest sources ?
>
> Regards,
> Khasim
>
> On 22 April 2016 at 03:12, <thakis@chromium.org> wrote:
>
>> Good to hear it works with upstream libjpeg-turbo. Can you try rolling us
>> forward then?
>>
>> https://codereview.chromium.org/1885373002/
>>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[khasim.mohammed, 2016-04-27 18:13:02]

Hi Nico,

I pulled the latest release of libjpeg-trubo
https://sourceforge.net/projects/libjpeg-turbo/files/1.4.90%20%281.5%20beta1%...

configured it locally
./configure --with-jpeg7 --without-mem-srcdst --without-arith-enc
--without-arith-dec

And build chromium as usual for Android, I found one simple error have
fixed it. And boot tested on ARMv8 hardware.

Should I regenerate these patches and submit it ?

Regards,
Khasim


On 23 April 2016 at 07:27, Khasim Syed Mohammed <khasim.mohammed@linaro.org>
wrote:

> Nico, can you please suggest on what we should do here, I am assuming
> these tasks
>
> a) Migrate to latest libjpeg-turbo, evaluate for ARMv7 and v8 with both
> GCC and Clang
> b) submit these patches to chromium
>
> Sorry I am new to this community so just trying to ensure I am doing the
> right thing.
>
> Regards,
> Khasim
>
>
> On 22 April 2016 at 09:04, Khasim Syed Mohammed <
> khasim.mohammed@linaro.org> wrote:
>
>> Sorry, I didn't get. Do you want me to build with complete libjpeg-turbo
>> latest sources ?
>>
>> Regards,
>> Khasim
>>
>> On 22 April 2016 at 03:12, <thakis@chromium.org> wrote:
>>
>>> Good to hear it works with upstream libjpeg-turbo. Can you try rolling us
>>> forward then?
>>>
>>> https://codereview.chromium.org/1885373002/
>>>
>>
>>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2016-04-27 20:39:29]

I mean can you update the libjpeg-turbo used by Chromium to the latest, if
things work there?

[Noel Gordon, 2016-04-29 02:35:05]

On 2016/04/27 18:13:02, khasim.mohammed wrote:
> Hi Nico,
> 
> I pulled the latest release of libjpeg-trubo
>
https://sourceforge.net/projects/libjpeg-turbo/files/1.4.90%20%281.5%20beta1%...
> 
> configured it locally
> ./configure --with-jpeg7 --without-mem-srcdst --without-arith-enc
> --without-arith-dec

Why use --with-jpeg7, and was the error you mentioned associated with that?

[khasim.mohammed, 2016-04-29 03:48:32]

On 29 April 2016 at 08:05, <noel@chromium.org> wrote:

> On 2016/04/27 18:13:02, khasim.mohammed wrote:
> > Hi Nico,
> >
> > I pulled the latest release of libjpeg-trubo
> >
>
>
https://sourceforge.net/projects/libjpeg-turbo/files/1.4.90%20%281.5%20beta1%...
> >
> > configured it locally
> > ./configure --with-jpeg7 --without-mem-srcdst --without-arith-enc
> > --without-arith-dec
>
> Why use --with-jpeg7, and was the error you mentioned associated with
> that?
>
> The first thing I tried was jpeg8, here is what happens.

*./configure  --with-jpeg8 --without-mem-srcdst*

/lib.unstripped/libgfx.cr.so" -Wl,-soname="libgfx.cr.so"
@"./libgfx.cr.so.rsp"
obj/third_party/libjpeg_turbo/libjpeg/jcinit.o: In function
`jinit_compress_master':
./out/Default/../../third_party/libjpeg_turbo/jcinit.c:47: undefined
reference to `jinit_arith_encoder'
obj/third_party/libjpeg_turbo/libjpeg/jdmaster.o: In function
`master_selection':
./out/Default/../../third_party/libjpeg_turbo/jdmaster.c:537: undefined
reference to `jinit_arith_decoder'

*./configure  --with-jpeg8 --without-mem-srcdst  --without-arith-enc*

-Xclang follow-macro-expansion -Wheader-hygiene -Wstring-conversion  -c
../../third_party/libjpeg_turbo/jcinit.c -o
obj/third_party/libjpeg_turbo/libjpeg/jcinit.o
../../third_party/libjpeg_turbo/jcinit.c:49:20: error: use of undeclared
identifier 'JERR_ARITH_NOTIMPL'
    ERREXIT(cinfo, JERR_ARITH_NOTIMPL);
                   ^
 I am not sure if there are libjpeg_turbo issues for ARM only, I wanted to
discuss this on libjpeg_turbo mailing list later today.

Any suggestions ?

Regards,
Khasim


https://codereview.chromium.org/1885373002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Noel Gordon, 2016-04-29 07:25:08]

Description was changed from

==========
Fix simd access for 64bit ARM neon clang compilation

This patch fixes the invalid operand access of
simd instructions on ARM64

compilation error example :

../../third_party/libjpeg_turbo/simd/jsimd_arm64_neon.S:1859:1:
note: while in macro instantiation
generate_jsimd_ycc_rgb_convert_neon rgb565, 16, 0, .4h, 0, .4h, 0, .4h, .8b
^
<instantiation>:4:31: error: invalid operand for instruction
    smlal v20.4s, v8.4h, v1.4h[2]

This patch helps in building 64bit browser with clang for ARM64

BUG : http://crbug.com/539781

Signed-off-by: Bernhard Rosenkränzer <bero@linaro.org>

BUG=539781
R=thakis@chromium.org
==========

to

==========
Fix simd access for 64bit ARM neon clang compilation

This patch fixes the invalid operand access of
simd instructions on ARM64

compilation error example :

../../third_party/libjpeg_turbo/simd/jsimd_arm64_neon.S:1859:1:
note: while in macro instantiation
generate_jsimd_ycc_rgb_convert_neon rgb565, 16, 0, .4h, 0, .4h, 0, .4h, .8b
^
<instantiation>:4:31: error: invalid operand for instruction
    smlal v20.4s, v8.4h, v1.4h[2]

This patch helps in building 64bit browser with clang for ARM64

BUG : http://crbug.com/539781

Signed-off-by: Bernhard Rosenkränzer <bero@linaro.org>

BUG=539781
R=thakis@chromium.org
==========

[Noel Gordon, 2016-04-29 07:29:48]

On 2016/04/29 03:48:32, khasim.mohammed wrote:
> On 29 April 2016 at 08:05, <mailto:noel@chromium.org> wrote:
> > >
> > > configured it locally
> > > ./configure --with-jpeg7 --without-mem-srcdst --without-arith-enc
> > > --without-arith-dec
> >
> > Why use --with-jpeg7, and was the error you mentioned associated with
> > that?
> >
> > The first thing I tried was jpeg8, here is what happens.
> 
> *./configure  --with-jpeg8 --without-mem-srcdst*

Maybe try

./configure \
  --without-mem-srcdst \
  --without-arith-enc \
  --without-arith-dec

>  I am not sure if there are libjpeg_turbo issues for ARM only, I wanted to
> discuss this on libjpeg_turbo mailing list later today.

Note Chromium does not use "configure" so if there are problems with it, then
agree, best to ask the turbo list.

Chromium would not want the --with-jpeg8 or --with-jpeg7 mind, we'd prefer to
stick the libjpeg6 interface that turbo provides.

[khasim.mohammed, 2016-04-29 07:36:35]

On 29 April 2016 at 12:59, <noel@chromium.org> wrote:

> On 2016/04/29 03:48:32, khasim.mohammed wrote:
> > On 29 April 2016 at 08:05, <mailto:noel@chromium.org> wrote:
> > > >
> > > > configured it locally
> > > > ./configure --with-jpeg7 --without-mem-srcdst --without-arith-enc
> > > > --without-arith-dec
> > >
> > > Why use --with-jpeg7, and was the error you mentioned associated with
> > > that?
> > >
> > > The first thing I tried was jpeg8, here is what happens.
> >
> > *./configure --with-jpeg8 --without-mem-srcdst*
>
> Maybe try
>
> ./configure \
> --without-mem-srcdst \
> --without-arith-enc \
> --without-arith-dec
>
>
Yes, this works as well. Will continue to use this.


> > I am not sure if there are libjpeg_turbo issues for ARM only, I wanted to
> > discuss this on libjpeg_turbo mailing list later today.
>
> Note Chromium does not use "configure" so if there are problems with it,
> then
> agree, best to ask the turbo list.
>
> Chromium would not want the --with-jpeg8 or --with-jpeg7 mind, we'd prefer
> to
> stick the libjpeg6 interface that turbo provides.
>
> ok, I would like to push these patches to chromium, I will do a git-rebase
and git-merge on my local setup and use gclient to push the patches. Is
this right approach ? Not sure how we do a third_party project update in
chromium, hence this question.


> https://codereview.chromium.org/1885373002/
>

Regards,
Khasim

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Noel Gordon, 2016-04-29 08:20:50]

On 2016/04/29 07:36:35, khasim.mohammed wrote:
> > ok, I would like to push these patches to chromium, I will do a git-rebase
> and git-merge on my local setup and use gclient to push the patches. Is
> this right approach ? 
> Not sure how we do a third_party project update in
> chromium, hence this question.

% git log -- third_party/libjpeg_turbo

outlines recent changes, and lead me to

https://codereview.chromium.org/1347093003/diff/20001/README.chromium

which explains how to update turbo.  We have a number of local patches that were
not accepted upstream, so we cherry-pick upstream changes if needed into
Chromium.

Ideally, if your "changes" are to turbo, then you should get them submitted
upstream first.  You said "changes".  Are there more changes needed for turbo?  
Are changes needed to upstream clang too?

[khasim.mohammed, 2016-04-29 09:31:40]

On 29 April 2016 at 13:50, <noel@chromium.org> wrote:

> On 2016/04/29 07:36:35, khasim.mohammed wrote:
> > > ok, I would like to push these patches to chromium, I will do a
> git-rebase
> > and git-merge on my local setup and use gclient to push the patches. Is
> > this right approach ?
> > Not sure how we do a third_party project update in
> > chromium, hence this question.
>
> % git log -- third_party/libjpeg_turbo
>
> outlines recent changes, and lead me to
>
> https://codereview.chromium.org/1347093003/diff/20001/README.chromium
>
> which explains how to update turbo. We have a number of local patches that
> were
> not accepted upstream, so we cherry-pick upstream changes if needed into
> Chromium.
>
> Ideally, if your "changes" are to turbo, then you should get them submitted
> upstream first. You said "changes". Are there more changes needed for
> turbo?
> Are changes needed to upstream clang too?
>
> No, the changes we need to build chromium with clang are in upstream but
not on chromium, so Nico suggested us to push latest changes from
libjpeg-turbo to chromium, the latest changes do work directly inside
chromium with just one fix,

--- /a/libjpeg-turbo/jerror.h 2016-04-21 13:49:00.947670694 +0530
+++ /b/libjpeg-turbo/jerror.h 2016-04-27 23:09:10.194676784 +0530
@@ -43,7 +43,7 @@ typedef enum {
 JMESSAGE(JMSG_NOMESSAGE, "Bogus message code %d") /* Must be first entry!
*/

 /* For maintenance convenience, list is alphabetical by message code name
*/
-#if JPEG_LIB_VERSION < 70
+#if JPEG_LIB_VERSION <= 70

I will push the changes for review and we can take it forward.

Thanks.

Regards,
Khasim

https://codereview.chromium.org/1885373002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[msarett, 2016-04-29 12:45:09]

msarett@google.com changed reviewers:
+ msarett@google.com

[msarett, 2016-04-29 12:45:09]

Yes I believe these fixes are in upstream.
https://github.com/libjpeg-turbo/libjpeg-turbo/commit/d70a5c12fcb72443483456a...
https://github.com/libjpeg-turbo/libjpeg-turbo/commit/62999d7708592b9c59c3d9c...

+1 for cherry picking from upstream and referencing those commits.

+2 for updating to the latest release version.
https://github.com/libjpeg-turbo/libjpeg-turbo/releases/tag/1.4.90

[Noel Gordon, 2016-04-29 13:07:08]

On 2016/04/29 09:31:40, khasim.mohammed wrote:
> libjpeg-turbo to chromium, the latest changes do work directly inside
> chromium with just one fix,
> 
> --- /a/libjpeg-turbo/jerror.h 2016-04-21 13:49:00.947670694 +0530
> +++ /b/libjpeg-turbo/jerror.h 2016-04-27 23:09:10.194676784 +0530
> @@ -43,7 +43,7 @@ typedef enum {
>  JMESSAGE(JMSG_NOMESSAGE, "Bogus message code %d") /* Must be first entry!
> */
> 
>  /* For maintenance convenience, list is alphabetical by message code name
> */
> -#if JPEG_LIB_VERSION < 70
> +#if JPEG_LIB_VERSION <= 70

Perhaps this error is due to using "./configure --with-jpeg7 ...". Properly
configured, JPEG_LIB_VERSION should equal 6b.

[khasim.mohammed, 2016-04-30 09:23:42]

On 29 April 2016 at 18:15, <msarett@google.com> wrote:

> Yes I believe these fixes are in upstream.
>
>
https://github.com/libjpeg-turbo/libjpeg-turbo/commit/d70a5c12fcb72443483456a...
>
>
https://github.com/libjpeg-turbo/libjpeg-turbo/commit/62999d7708592b9c59c3d9c...
>
> +1 for cherry picking from upstream and referencing those commits.
>
> +2 for updating to the latest release version.
> https://github.com/libjpeg-turbo/libjpeg-turbo/releases/tag/1.4.90
>
> Have submitted the patches : https://codereview.chromium.org/1934113002,
not sure if this was the right thing to do, here is how I have achieved the
merge,

Have synced to latest on chromium for libjpeg_turbo
Created a branch
Did git remote add to chromium git tree
git merge -s recursive -Xtheirs 1.4.90

There was conflict, have resolved it in separate patch
Copied the config.h and jconfig from public project and included this in
separate patch.


> https://codereview.chromium.org/1885373002/
>

Regards,
Khasim

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[khasim.mohammed, 2016-05-09 06:58:01]

On 2016/04/30 09:23:42, khasim.mohammed wrote:
> On 29 April 2016 at 18:15, <mailto:msarett@google.com> wrote:
> 
> > Yes I believe these fixes are in upstream.
> >
> >
>
https://github.com/libjpeg-turbo/libjpeg-turbo/commit/d70a5c12fcb72443483456a...
> >
> >
>
https://github.com/libjpeg-turbo/libjpeg-turbo/commit/62999d7708592b9c59c3d9c...
> >
> > +1 for cherry picking from upstream and referencing those commits.
> >
> > +2 for updating to the latest release version.
> > https://github.com/libjpeg-turbo/libjpeg-turbo/releases/tag/1.4.90
> >
> > Have submitted the patches : https://codereview.chromium.org/1934113002,
> not sure if this was the right thing to do, here is how I have achieved the
> merge,
> 
> Have synced to latest on chromium for libjpeg_turbo
> Created a branch
> Did git remote add to chromium git tree
> git merge -s recursive -Xtheirs 1.4.90
> 
> There was conflict, have resolved it in separate patch
> Copied the config.h and jconfig from public project and included this in
> separate patch.
> 
> 
> > https://codereview.chromium.org/1885373002/
> >

This patch is not required as we have migrated to latest libjpeg turbo 1.4.9.

> 
> Regards,
> Khasim
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.