934174 - Optimize the interpreter's int32 add

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Dan Gohman [:sunfish]]

Attachment: interp-add.patch

This patch optimizes the int32 + int32 path to do an int32 add, to avoid going through floating-point in the common case.

Comment 1

[Luke Wagner [:luke]]

Comment on attachment 826365 [details] [diff] [review]
interp-add.patch

Makes sense.

Comment 2

[Luke Wagner [:luke]]

As long as you're optimizing interp add and we have SafeAdd to abstract the grossness, you might consider trying alternative overflow tests:
  http://hg.mozilla.org/mozilla-central/rev/9c869e64ee26#l89.2964
I suppose it depends on how smart the compiler is about 64-bit int addition (my experience looking at GCC output a few years ago left me unimpressed).  (Yes, that code also has the signed-overflow UB you just reported on SafeAdd; uint32_t should work.)

Alternatively, I keep expecting there to be some compiler builtin
  bool __add_with_overflow(int32 lhs,int32 rhs,int32 *result)
that returns whether the addition overflowed so that we could take advantage of the overflow bit that the compiler already sets, but every time I go looking I can't find one.  Do you know of one?

Comment 3

[Luke Wagner [:luke]]

s/the compiler already sets/the processor already sets/

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Luke Wagner [:luke] from comment #2)
> As long as you're optimizing interp add and we have SafeAdd to abstract the
> grossness, you might consider trying alternative overflow tests:
>   http://hg.mozilla.org/mozilla-central/rev/9c869e64ee26#l89.2964
> I suppose it depends on how smart the compiler is about 64-bit int addition
> (my experience looking at GCC output a few years ago left me unimpressed). 

Do you remember what sort of problems you saw?

> Alternatively, I keep expecting there to be some compiler builtin
>   bool __add_with_overflow(int32 lhs,int32 rhs,int32 *result)
> that returns whether the addition overflowed so that we could take advantage
> of the overflow bit that the compiler already sets, but every time I go
> looking I can't find one.  Do you know of one?

clang might have it; I know they have one for multiplication.  GCC does not.

Comment 5

[Luke Wagner [:luke]]

(In reply to Nathan Froyd (:froydnj) from comment #4)
> Do you remember what sort of problems you saw?

IIRC, it was on x86 (x64 was fine, as you might hope :) and I just noticed a ton of what seemed like unnecessary spilling as the int64 values got moved around.

Comment 6

[Luke Wagner [:luke]]

Also, and this might have been an issue with structs, not int64, but after the move to 64-bit js::Value (which was/is a 64-bit struct), we found more than a few perf regressions where the fix was to transform a loop of the form:

  for (int i = 0; i < N; i++)
    dst[i] = src[i]

into

  for (Value *dstp = dst, *srcp = src, *srcend = src + N; srcp != srcend; srcp++, dstp++)
    *dstp = *srcp;

because either the struct or 64-bit-ness was throwing off all the normal optimizations that usually allow us to write the former w/o loss of performance.

Comment 7

[Dan Gohman [:sunfish]]

Yeah, clang does have builtin functions for these. And yeah, that thing with the xors looks like a pretty good alternative. Next time I take another look at interpreter performance, I'll take a look, though I don't know when that'll be.

Comment 8

[Dan Gohman [:sunfish]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0cad525a6b4

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d0cad525a6b4