ARM Skia NEON patches - 04 - Clean SkFixed / SkLONGLONG

ARM Skia NEON patches - 04 - Clean SkFixed / SkLONGLONG

It removes SkLONGLONG and uses int64_t to implement the  SkFixed
operations for which a SkLONGLONG version existed. It also
removes the 32 bit version that are being replaced.

BUG=

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

[djsollen, 2013-07-15 12:16:12]

https://codereview.chromium.org/18539004/diff/1/src/core/SkMath.cpp
File src/core/SkMath.cpp (left):

https://codereview.chromium.org/18539004/diff/1/src/core/SkMath.cpp#oldcode134
src/core/SkMath.cpp:134: #elif defined(SkLONGLONG)
in include/core/SkPostConfig.h we define SkLONGLONG on both windows and mac.

[kevin.petit.not.used.account, 2013-07-15 13:51:24]

https://codereview.chromium.org/18539004/diff/1/src/core/SkMath.cpp
File src/core/SkMath.cpp (left):

https://codereview.chromium.org/18539004/diff/1/src/core/SkMath.cpp#oldcode134
src/core/SkMath.cpp:134: #elif defined(SkLONGLONG)
On 2013/07/15 12:16:12, djsollen wrote:
> in include/core/SkPostConfig.h we define SkLONGLONG on both windows and mac.

Yes but in this case, and unless I've missed something, the code
at include/core/SkFixed.h:175 is used. The comment just above may
also be worth a look.

[kevin.petit.not.used.account, 2013-07-25 18:19:05]

ping

[djsollen, 2013-07-29 16:42:10]

I follow the logic and can see that the code as is won't be invoked, but not
sure what the benefit of removing this code is.  If in the future someone called
into this "portable" function then shouldn't we handle the SkLONGLONG case
properly?

Mike, do you have any opinion on this code since I think you were the original
author?

[reed1, 2013-07-29 16:47:37]

If we want to do some cleanup around fixed, I think I'd like to see something
more sweeping...

1. I presume all toolchains support int64_t now.
2. I think compilers now are very efficient with 64bit math, even on 32bit
architectures.

If these two are true, perhaps we can/should remove SkLONGLONG entirely, and
just assume that, in the absence of any assembly override, casting to int64_t
and doing straight-forward math is fine.

[kevin.petit.not.used.account, 2013-08-06 12:20:52]

On 2013/07/29 16:47:37, reed1 wrote:

> If these two are true

It is true for ARM and honestly I haven't seen a toolchain that doesn't support
(correctly) int64_t in a long time (unless you want to support small
processors).

> perhaps we can/should remove SkLONGLONG entirely, and
> just assume that, in the absence of any assembly override, casting to int64_t
> and doing straight-forward math is fine.

That's what my second version of the patch does. I presume it should be tested
in priority on Windows and Mac platforms, which I cannot do.

[reed1, 2013-08-07 13:59:05]

I'm glad we're removing the conditionalness of SkLONGLONG. However, the CL seems
to have just removed all of that code. Is that because we are slower using
longlong than using 32bit ints?

https://codereview.chromium.org/18539004/diff/7001/src/core/SkMath.cpp
File src/core/SkMath.cpp (left):

https://codereview.chromium.org/18539004/diff/7001/src/core/SkMath.cpp#oldcod...
src/core/SkMath.cpp:116: return static_cast<SkFixed>((SkLONGLONG)a * b >> 16);
You have removed the longlong version. Is that because it is slower than the
32bit version below?

[kevin.petit.not.used.account, 2013-08-07 15:42:56]

On 2013/08/07 13:59:05, reed1 wrote:
> I'm glad we're removing the conditionalness of SkLONGLONG. However, the CL
seems
> to have just removed all of that code. Is that because we are slower using
> longlong than using 32bit ints?

My initial patch removed that because it was dead code (see above).

My second patch completely missed the "casting to int64_t and doing
straight-forward math is fine" part of your suggestion. The 32bit portable
version, at least on ARM, will be slower, by far.

The third patch I just uploaded should be more sensible. It removes SkLONGLONG
and uses int64_t to implement the SkFixed operations for which a SkLONGLONG
version existed. It also removes the 32 bit version that are being replaced.

While doing this I also noticed a few places where Sk64 could be replaced with
int64_t arithmetic. For example, SkMulDiv shouldn't need two(!!) function calls
to do some basic maths. That could be a next step if you're happy with the third
patch.

[kevin.petit.not.used.account, 2013-08-08 10:58:41]

It seems I'm not able to use the trybots (which I totally understand). I'm just
making this comment so you know the failures above (patch set 3) are probably
not real. Clicking on the trybot results points me to the review page rather
than to actual results.

[reed1, 2013-08-12 14:41:25]

lgtm

[kevin.petit.not.used.account, 2013-08-14 11:06:36]

I've just edited the description to better reflect what it now does. Looks like
this one is just waiting to be commited :-).

[commit-bot: I haz the power, 2013-08-14 11:33:20]

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

[commit-bot: I haz the power, 2013-08-14 11:44:00]

Change committed as 10705

[robertphillips, 2013-08-15 13:44:12]

Reverted in r10729 due to 1000+ linux_layout failures
(http://build.chromium.org/p/tryserver.chromium/builders/linux_layout_rel/buil...)