Page MenuHomePhabricator

[Clang] Add elementwise saturated add/sub builtins
ClosedPublic

Authored by RKSimon on Jan 21 2022, 8:51 AM.

Details

Summary

This patch implements __builtin_elementwise_add_sat and __builtin_elementwise_sub_sat builtins

These map to the add/sub saturated math intrinsics described here:
https://llvm.org/docs/LangRef.html#saturation-arithmetic-intrinsics

With this in place we should then be able to replace the x86 SSE adds/subs intrinsics with these generic variants - it looks like other targets should be able to use these as well (arm/aarch64/webassembly all have similar examples in cgbuiltin).

Diff Detail

Event Timeline

RKSimon created this revision.Jan 21 2022, 8:51 AM
RKSimon requested review of this revision.Jan 21 2022, 8:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 21 2022, 8:51 AM
Herald added a subscriber: aheejin. · View Herald Transcript
RKSimon edited the summary of this revision. (Show Details)Jan 21 2022, 1:21 PM
craig.topper added inline comments.Jan 21 2022, 11:12 PM
clang/docs/LanguageExtensions.rst
549

Not sure if I'm reading this right due to the columns, but is "unsigned" missing after the "signed or"

clang/include/clang/Basic/Builtins.def
655

I don't think these are in alphabetical order. ceil is after min. Looks more like they were grouped by similar operations.

clang/lib/Sema/SemaChecking.cpp
2255

clang-format

RKSimon updated this revision to Diff 402313.Jan 23 2022, 4:12 AM

address feedback

fhahn added inline comments.Jan 28 2022, 1:50 AM
clang/test/CodeGen/builtins-elementwise-math.c
122

It might be good to have tests where one argument is signed and the other unsigned as well. Those appear to be missing for other builtins as well unfortunately.

RKSimon updated this revision to Diff 404013.Jan 28 2022, 6:56 AM

rebase and add signed/unsigned integer mismatch tests

aaron.ballman added inline comments.Jan 28 2022, 7:44 AM
clang/docs/LanguageExtensions.rst
549

This reads strangely to me as well. "..., clamped to the range of signed or integer types unsigned values representable by.."

clang/lib/CodeGen/CGBuiltin.cpp
3157–3184

Almost all of this logic is shared (except for picking the intrinsic), should it be combined?

clang/test/CodeGen/builtins-elementwise-math.c
122

I think it'd be good to have some codegen tests for _BitInt to make sure the behavior is correct.

RKSimon updated this revision to Diff 406264.Feb 6 2022, 10:09 AM

rebase, simplify description in documentation and add _BitInt test coverage

aaron.ballman accepted this revision.Feb 7 2022, 9:28 AM

Aside from some nits, this LGTM.

clang/docs/LanguageExtensions.rst
549

This still seems unaddressed.

clang/lib/CodeGen/CGBuiltin.cpp
3157–3184

Same with this one.

This revision is now accepted and ready to land.Feb 7 2022, 9:28 AM
RKSimon added inline comments.Feb 7 2022, 9:41 AM
clang/docs/LanguageExtensions.rst
549

I rephrased it - what did you have in mind?

aaron.ballman added inline comments.Feb 7 2022, 9:47 AM
clang/docs/LanguageExtensions.rst
549

I think what's been throwing me off is "range of integer types signed or unsigned values", but I don't think signed or unsigned really matters here, so I tried to reword it a bit. Does this work for you?

return the sum of x and y, clamped to the range of representable values for the integer type.

(Similar below for difference).

This revision was automatically updated to reflect the committed changes.