Page MenuHomePhabricator

Add AddInt63Overflow and SubInt63Overflow
ClosedPublic

Authored by tobias_ruck on Aug 25 2021, 04:44.

Details

Reviewers
deadalnix
Fabien
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rABCd91e8262c060: Add AddInt63Overflow and SubInt63Overflow
Summary

Computes 63+sign-bit addition and subtraction with overflow checks. It uses builtin_saddll_overflow and builtin_ssubll_overflow if available and falls back to AddInt63OverflowEmulated and SubInt63OverflowEmulated otherwise.

Allows 63+sign-bit integers in Script down the line.

Test Plan

ninja check

Diff Detail

Event Timeline

Owners added a reviewer: Restricted Owners Package.Aug 25 2021, 04:44
Fabien requested changes to this revision.Aug 30 2021, 12:13
Fabien added a subscriber: Fabien.
Fabien added inline comments.
src/config/CMakeLists.txt
59 ↗(On Diff #29541)

You will benefit from using the default ${ARGN} variable and string(JOIN) here, like:

string(JOIN ", " ARGS ${ARGN})

This will let you pass any number of argument, including none.

This revision now requires changes to proceed.Aug 30 2021, 12:13
majcosta added inline comments.
src/script/intmath.h
39–48 ↗(On Diff #29541)

an API that returns false when the computation is performed successfully and true when it isn't is a bit weird, you could RII"R":

Update detection of built-ins.

deadalnix requested changes to this revision.Sep 14 2021, 23:26
deadalnix added inline comments.
src/script/intmath.h
41

You need to check for the size of a long long.

This revision now requires changes to proceed.Sep 14 2021, 23:26

Add comment for the int64_t == long long int assertion.

Fabien added inline comments.
src/config/CMakeLists.txt
104 ↗(On Diff #29975)

You can make it a macro instead of a function

deadalnix requested changes to this revision.Sep 15 2021, 20:39

Ok, nevermind, the assumptions are there, but we have a file for that. This avoids baking in contradictory assumptions.

The test however needs to be improved so that correct behavior is obvious.

src/script/intmath.h
14 ↗(On Diff #29975)

You should move this in assumption.h

src/test/intmath_tests.cpp
140 ↗(On Diff #29975)

You should stick these checks just after the corresponding emulated check, so that it can be asserted that both return the same result easily.

This revision now requires changes to proceed.Sep 15 2021, 20:39

Verify [Add|Sub]Int63OverflowEmulated and [Add|Sub]Int63Overflow overflow for the same operands and produce the same result for for non-overflowing operands.

deadalnix requested changes to this revision.Mon, Sep 20, 14:34
deadalnix added inline comments.
src/test/intmath_tests.cpp
86 ↗(On Diff #29987)

This has the same problem as the previous code. You want the code to be the same for both by construct not by cutting and pasting.

This revision now requires changes to proceed.Mon, Sep 20, 14:34

Add CheckArithmeticResult to intmat_tests for less copy-pasta

This revision is now accepted and ready to land.Thu, Sep 23, 14:03