Page MenuHomePhabricator

[Clang] Add elementwise abs builtin.
ClosedPublic

Authored by fhahn on Oct 18 2021, 4:49 AM.

Details

Summary

This patch implements __builtin_elementwise_abs as specified in
D111529.

Diff Detail

Event Timeline

fhahn created this revision.Oct 18 2021, 4:49 AM
fhahn requested review of this revision.Oct 18 2021, 4:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 18 2021, 4:49 AM
fhahn updated this revision to Diff 380411.Oct 18 2021, 8:26 AM

polish tests a bit

craig.topper added inline comments.Oct 18 2021, 10:41 AM
clang/lib/CodeGen/CGBuiltin.cpp
3109

Did we discuss that this is different than __builtin_abs which is undefined for INT_MIN?

craig.topper added inline comments.Oct 18 2021, 11:35 AM
clang/lib/CodeGen/CGBuiltin.cpp
3109

I don't think we add nsw to vector signed integer subtract, so I guess using false here is consistent.

fhahn updated this revision to Diff 380618.Oct 19 2021, 2:29 AM

Pass is_int_min_poison=true to llvm.abs.* to reflect latest update to the specification to make taking abs of the most negative integer undefined.

fhahn marked 2 inline comments as done.Oct 19 2021, 2:33 AM
fhahn added inline comments.
clang/lib/CodeGen/CGBuiltin.cpp
3109

Did we discuss that this is different than __builtin_abs which is undefined for INT_MIN?

So far it was not explicitly called out in the definition. I think we should explicitly make it undefined, so I updated the spec in D111529 and passed true here.

What's the rationale for making abs undefined on the minimum value? AFAIK every actual simd implementation defines the result and they agree (and even if one didn't, it would be pretty easy to get the "right" result. Introducing UB here just seems like punishing users for no reason.

fhahn updated this revision to Diff 380756.Oct 19 2021, 12:35 PM

What's the rationale for making abs undefined on the minimum value? AFAIK every actual simd implementation defines the result and they agree (and even if one didn't, it would be pretty easy to get the "right" result. Introducing UB here just seems like punishing users for no reason.

Thanks! As making it UB does not really help with lowering I went back to allowing INT_MIN and updated the spec to explicitly spell out the behavior.

scanon accepted this revision.Oct 19 2021, 1:02 PM

Two minor questions, but also LGTM as is.

clang/lib/Sema/SemaChecking.cpp
16538

Given that I expect this particular test to occur fairly frequently, maybe worth abstracting into some sort of get-elementwise-type operation.

clang/test/Sema/builtins-elementwise-math.c
25

For the purposes of C++ templates it might be nice to allow abs on unsigned (as the identity function). I don't have strong feelings though, and a library wrapping the builtins can do this themselves.

This revision is now accepted and ready to land.Oct 19 2021, 1:02 PM
fhahn updated this revision to Diff 380947.Oct 20 2021, 7:25 AM

Use checkMathBuiltinElementType helper added in D111985.

fhahn marked 2 inline comments as done.Oct 20 2021, 7:34 AM
fhahn added inline comments.
clang/lib/Sema/SemaChecking.cpp
16538

moved out to a helper in the previous patch in the series, D111985

clang/test/Sema/builtins-elementwise-math.c
25

yeah that would be an option. I don't have any strong feelings either

fhahn updated this revision to Diff 382226.Oct 26 2021, 1:54 AM
fhahn marked 2 inline comments as done.

Rebased after recent changes in D111985: changes the return type of SemaBuiltinElementwiseMathOneArg to bool and drop the CallResult argument. Also added tests with const, typedef'ed and variables with address space

aaron.ballman added inline comments.Oct 26 2021, 5:21 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
8748

I feel like we must already have a diagnostic that covers this case...

clang/lib/Sema/SemaChecking.cpp
16548

Should this type undergo the usual promotions?

clang/test/Sema/builtins-elementwise-math.c
25

I was wondering about this myself, but also don't have strong opinions.

fhahn updated this revision to Diff 382376.Oct 26 2021, 9:49 AM
fhahn marked 2 inline comments as done.

Rebased and updated to use err_builtin_invalid_arg_type.

clang/include/clang/Basic/DiagnosticSemaKinds.td
8748

Updated to use err_builtin_invalid_arg_type.

clang/lib/Sema/SemaChecking.cpp
16548

I'm not sure, but given that we only have a single argument then wouldn't it be sufficient to avoid promotion? I don't think promotion to wider types would impact the results of the provided builtins.

aaron.ballman added inline comments.Oct 26 2021, 11:49 AM
clang/lib/Sema/SemaChecking.cpp
16548

You set the type of the call to be the type of the argument, which means passing in a const int will result in a const int that's observable and probably unexpected. e.g. this will fail,

const int a = -12;
static_assert(!std::is_const_v<decltype(__builtin_elementwise_abs(a))>);

(This can come up with overload resolution or in template specializations.)

fhahn updated this revision to Diff 382443.Oct 26 2021, 1:17 PM

Apply UsualUnaryConversions to input argument.

fhahn marked an inline comment as done.Oct 26 2021, 1:18 PM
fhahn added inline comments.
clang/lib/Sema/SemaChecking.cpp
16548

Ah right. I originally was planning on just using getUnqualifiedType, but for consistency it seems better to just apply the usual unary conversions. I also added the test above and a codegen tests with short.

fhahn updated this revision to Diff 382445.Oct 26 2021, 1:19 PM
fhahn marked an inline comment as done.

add comment to arg of Diag message

aaron.ballman accepted this revision.Oct 27 2021, 9:55 AM

LGTM, thank you!

This revision was automatically updated to reflect the committed changes.