Michaël,
>> I would put back unlikely() on overflow tests, as there are indeed unlikely
>> to occur and it may help some compilers, and cannot be harmful. It also
>> helps the code reader to know that these path are not expected to be taken
>> often.
>
> Hm. I don't agree about that part, and the original signed portions
> don't do that. I think that we should let the callers of the routines
> decide if a problem is unlikely to happen or not as we do now.
Hmmm. Maybe inlining propates them, but otherwise they make sense for a
compiler perspective.
> I am still not convinced that this module is worth the extra cycles to
> justify its existence though.
They allow to quickly do performance tests, for me it is useful to keep it
around, but you are the committer, you do as you feel.
>> [...]
>> There is no doubt that dividing 64 bits integers is a very bad idea, at
>> least on my architecture!
>
> That's surprising. I cannot reproduce that.
It seems to me that somehow you can, you have a 5 to 18 seconds drop
below. There are actual reasons why some processors are more expensive
than others, it is not just marketing:-)
> 2-1) mul:
> - built-in: 5.1s
> - fallback (with uint128): 8.0s
> - fallback (without uint128): 18.1s
> So, the built-in option is always faster, and keeping the int128 path
> if available for the multiplication makes sense, but not for the
> subtraction and the addition.
Yep.
> I am wondering if we should review further the signed part for add and
> sub, but I'd rather not touch it in this patch.
The signed overflows are trickier even, I have not paid attention to the
fallback code. I agree that it is better left untouched for know.
> If you have done any changes on my previous patch, or if you have a
> script to share I could use to attempt to reproduce your results, I
> would be happy to do so.
Hmmm. I did manual tests really. Attached a psql script replicating them.
# with builtin overflow detection
sh> psql < oc.sql
NOTICE: int 16 mul: 00:00:02.747269 # slow
NOTICE: int 16 add: 00:00:01.83281
NOTICE: int 16 sub: 00:00:01.8501
NOTICE: uint 16 mul: 00:00:03.68362 # slower
NOTICE: uint 16 add: 00:00:01.835294
NOTICE: uint 16 sub: 00:00:02.136895 # slow
NOTICE: int 32 mul: 00:00:01.828065
NOTICE: int 32 add: 00:00:01.840269
NOTICE: int 32 sub: 00:00:01.843557
NOTICE: uint 32 mul: 00:00:02.447052 # slow
NOTICE: uint 32 add: 00:00:01.849899
NOTICE: uint 32 sub: 00:00:01.840773
NOTICE: int 64 mul: 00:00:01.839051
NOTICE: int 64 add: 00:00:01.839065
NOTICE: int 64 sub: 00:00:01.838599
NOTICE: uint 64 mul: 00:00:02.161346 # slow
NOTICE: uint 64 add: 00:00:01.839404
NOTICE: uint 64 sub: 00:00:01.838549
DO
> So, do you have more comments?
No other comments.
--
Fabien.
