751814 - [Skia] Fixes for ARMv6+ and ARMv4T

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Mike Hommey [:glandium]]

In gfx/skia/src/opts/SkBlitRow_opts_arm.cpp, smulbb is used when it's not supported on ARMv4T or ARMv5T. (gfx/skia/include/core/SkMath.h has guards for smulbb use, but they aren't in SkBlitRow_opts_arm.cpp)

In gfx/skia/src/opts/SkBitmapProcState_opts_arm.cpp, Some code is conditional on the ARM architecture being >= 6, but the macro it uses to do that isn't defined, so in practice, this code is not used on ARMv6 and ARMv7.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Various Skia fixes for ARMv4T and ARMv6+

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 620967 [details] [diff] [review]
Various Skia fixes for ARMv4T and ARMv6+

Review of attachment 620967 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I'm not competent for this review. Neither with the ARM platform detection, nor with ARM assembly. Try derf?

Comment 3

[Timothy B. Terriberry (:derf)]

Comment on attachment 620967 [details] [diff] [review]
Various Skia fixes for ARMv4T and ARMv6+

Review of attachment 620967 [details] [diff] [review]:
-----------------------------------------------------------------

The asm itself looks fine. I don't think your #if check is quite right (it misses some cases that gcc has used over the years).

But there are two things I want to know before I can r+ this:

1) gfx/skia has an update.sh and a list of patches that we apply to the upstream source. I assume that these changes need to be added to that script? If there's some other procedure we use for skia, or we're not really maintaining this script, let me know.

2) This only does compile-time detection. Our policy elsewhere in the tree is to do run-time detection. From my brief glance, it doesn't look like skia supports run-time detection at all. Is this correct? What's the story here?

::: gfx/skia/src/opts/SkBlitRow_opts_arm.cpp
@@ +673,5 @@
>  
>                    /* dst1_scale and dst2_scale*/
>                    "lsr    r9, r5, #24                \n\t" /* src >> 24 */
>                    "lsr    r10, r6, #24               \n\t" /* src >> 24 */
> +#if defined(__ARM_ARCH_4T__) || defined(__ARM_ARCH_5T__)

I think you want
#if __ARM_ARCH__ < 5 || __ARM_ARCH__ == 5 && !(defined(__ARM_ARCH_5E__) || defined(__ARM_ARCH_5TE__) || defined(__ARM_ARCH_5TEJ__))

Not that we plan to support less than ARMv4, but AFAIK we don't even support less than ARMv6. See the tests I added to xpcom/glue/arm.h for reference.

Comment 4

[Mike Hommey [:glandium]]

(In reply to Timothy B. Terriberry (:derf) from comment #3)
> 2) This only does compile-time detection. Our policy elsewhere in the tree
> is to do run-time detection. From my brief glance, it doesn't look like skia
> supports run-time detection at all. Is this correct? What's the story here?

AFAIK, it doesn't. I am not upstream so I can't tell you if it's expected to be supported at some point or not.

> ::: gfx/skia/src/opts/SkBlitRow_opts_arm.cpp
> @@ +673,5 @@
> >  
> >                    /* dst1_scale and dst2_scale*/
> >                    "lsr    r9, r5, #24                \n\t" /* src >> 24 */
> >                    "lsr    r10, r6, #24               \n\t" /* src >> 24 */
> > +#if defined(__ARM_ARCH_4T__) || defined(__ARM_ARCH_5T__)
> 
> I think you want
> #if __ARM_ARCH__ < 5 || __ARM_ARCH__ == 5 && !(defined(__ARM_ARCH_5E__) ||
> defined(__ARM_ARCH_5TE__) || defined(__ARM_ARCH_5TEJ__))

I used that line because there's another place using smulbb using the same test. We could change both, I guess. Also note __ARM_ARCH__ doesn't exist, which the point of the other hunk, so it would need to be in a header. Which one would be the most appropriate?

> Not that we plan to support less than ARMv4, but AFAIK we don't even support
> less than ARMv6. See the tests I added to xpcom/glue/arm.h for reference.

Actually, Debian does support ARMv4T.

Comment 5

[Timothy B. Terriberry (:derf)]

(In reply to Mike Hommey [:glandium] from comment #4)
> I used that line because there's another place using smulbb using the same
> test. We could change both, I guess. Also note __ARM_ARCH__ doesn't exist,
> which the point of the other hunk, so it would need to be in a header. Which
> one would be the most appropriate?

I'd make a new SKUtils_opts_arm.h (like the one for SSE2) and put them in there.

The reason I didn't like the original test is that if you don't know the architecture (because the appropriate #defines are not set or you're simply testing for the wrong ones), the default will be to use SMULBB, not the safer MUL. I think you should change it in both places. In fact, I'd probably put the whole test in that header and define a new macro specifically to test for compiler support for the EDSP instructions. That would encourage everyone (I'm assuming you're sending these patches upstream) to use the same test instead of cutting and pasting.

> > Not that we plan to support less than ARMv4, but AFAIK we don't even support
> > less than ARMv6. See the tests I added to xpcom/glue/arm.h for reference.
> 
> Actually, Debian does support ARMv4T.

Hmm, that is a good point. I wasn't sure if that had been dropped with the new armhf port, but I guess the eabi armel port is still being shipped. If that version is compiled specifically for ARMv4T, then I can see the problem.

Comment 6

[Mike Hommey [:glandium]]

Attachment: Various fixes for ARM without EDSP and ARMv6+

Also sent upstream on https://codereview.appspot.com/6195072

Should I put a reviewer there?

Comment 7

[Mike Hommey [:glandium]]

Attachment: Various Skia fixes for ARM without EDSP and ARMv6+

With update.sh changes.

Comment 8

[Timothy B. Terriberry (:derf)]

(In reply to Mike Hommey [:glandium] from comment #6)
> Should I put a reviewer there?

I don't know who the appropriate upstream reviewer is. If you don't get a response, maybe mattwoodrow or one of the other #gfx folks has some idea.

Comment 9

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b809c95bd21

Comment 10

[Matt Woodrow (:mattwoodrow)]

CC'ing george, as he's had previous experience upstreaming Skia patches.

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2b809c95bd21