Skip to content

Fix some sources of signed integer overflow in the compiler#7231

Merged
abadams merged 4 commits intomainfrom
abadams/fix_7229
Dec 13, 2022
Merged

Fix some sources of signed integer overflow in the compiler#7231
abadams merged 4 commits intomainfrom
abadams/fix_7229

Conversation

@abadams
Copy link
Copy Markdown
Member

@abadams abadams commented Dec 12, 2022

Also, use compiler intrinsics when possible to handle overflow, as it generates faster code.

Fixes #7229

@abadams
Fix some sources of signed integer overflow in the compiler
33cf23e 
Also, use compiler intrinsics when possible to handle overflow, as it
generates faster code.
steven-johnson
steven-johnson approved these changes Dec 12, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@steven-johnson steven-johnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I will pull this into Google for testing

Comment thread src/ModulusRemainder.cpp Outdated
Comment thread src/Util.h Outdated
@steven-johnson
Copy link
Copy Markdown
Contributor

steven-johnson commented Dec 12, 2022

src\CMakeFiles\Halide.dir\Util.cpp.obj /Fdsrc\CMakeFiles\Halide.dir\ /FS -c D:\build_bot\worker\halide-testbranch-main-llvm16-x86-32-windows-cmake\halide-source\src\Util.cpp
D:\build_bot\worker\halide-testbranch-main-llvm16-x86-32-windows-cmake\halide-source\src\Util.cpp(529): error C3861: '__builtin_saddll_overflow': identifier not found
D:\build_bot\worker\halide-testbranch-main-llvm16-x86-32-windows-cmake\halide-source\src\Util.cpp(557): error C3861: '__builtin_ssubll_overflow': identifier not found
D:\build_bot\worker\halide-testbranch-main-llvm16-x86-32-windows-cmake\halide-source\src\Util.cpp(597): error C3861: '__builtin_smulll_overflow': identifier not found

@abadams
Copy link
Copy Markdown
Member Author

abadams commented Dec 12, 2022

src\CMakeFiles\Halide.dir\Util.cpp.obj /Fdsrc\CMakeFiles\Halide.dir\ /FS -c D:\build_bot\worker\halide-testbranch-main-llvm16-x86-32-windows-cmake\halide-source\src\Util.cpp D:\build_bot\worker\halide-testbranch-main-llvm16-x86-32-windows-cmake\halide-source\src\Util.cpp(529): error C3861: '__builtin_saddll_overflow': identifier not found D:\build_bot\worker\halide-testbranch-main-llvm16-x86-32-windows-cmake\halide-source\src\Util.cpp(557): error C3861: '__builtin_ssubll_overflow': identifier not found D:\build_bot\worker\halide-testbranch-main-llvm16-x86-32-windows-cmake\halide-source\src\Util.cpp(597): error C3861: '__builtin_smulll_overflow': identifier not found

Oops, it's _MSC_VER, not __MSC_VER

@steven-johnson
Copy link
Copy Markdown
Contributor

steven-johnson commented Dec 12, 2022

Still failing ~every Windows test

@steven-johnson
Copy link
Copy Markdown
Contributor

steven-johnson commented Dec 12, 2022

Google integrate looks clean, just need to figure out Windows

@abadams abadams merged commit 1a4a469 into main Dec 13, 2022
ardier pushed a commit to ardier/Halide-mutation that referenced this pull request Mar 3, 2024
@abadams @ardier
Fix some sources of signed integer overflow in the compiler (halide#7231
f8b2518 
)

* Fix some sources of signed integer overflow in the compiler

Also, use compiler intrinsics when possible to handle overflow, as it
generates faster code.

* Fix msvc macro

* Must use result

* Actually perform the requested operation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@steven-johnson steven-johnson steven-johnson approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ASAN reports signed-integer-overflow in correctness_intrinsics and correctness_code_explosion

2 participants

@abadams @steven-johnson