Page MenuHomePhabricator

[Builtins] Support ext_vector_type args for __builtin_fminf.
Changes PlannedPublic

Authored by fhahn on Aug 27 2021, 10:12 AM.

Details

Summary

At the moment, the various math builtins only accept the scalar types
defined by libm.

This patch extends __builtin_fminf to also accept ext_vector_type
arguments that can be converted to an ext_vector_type with float as
element type.

If this direction makes sense, I am planning to also convert a few other
builtins that can be extended to vectors directly, like fmax.

In addition to vector types, it would be also good to support matrix
types as follow up I think.

I tried to make sure the same error messages are emitted for the scalar
variant of __builtin_fminf, but there's a difference for the (vector,
float) variant. I still need to dig into where this is coming from.

Diff Detail

Event Timeline

fhahn requested review of this revision.Aug 27 2021, 10:12 AM
fhahn created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptAug 27 2021, 10:12 AM
fhahn edited the summary of this revision. (Show Details)Aug 27 2021, 10:20 AM

This brings Clang more in line with GCC, which already supports math builtins with ext_vector_type arguments.

This is unfortunately not correct. When I tried this with GCC the ext_vector_type attribute was dropped and it fell back to float.

I think I would want some level of examination/analysis as to whether we want to do this with all of the vector-types, instead of just the ext_vector_type.

clang/lib/Sema/SemaChecking.cpp
16544

Doing this as ExprResult(true) is absurdly jarring.

16554

I wonder if this should just deal in VectorType, which is the type that intends to emulate the GCC vector types. My understanding is ExtVectorType is for the clang-extended vector types.

16575

What does this comment come from? Is this supposed to be a 'FIXME'? I don't see any other matrix stuff here.

fhahn updated this revision to Diff 369135.Aug 27 2021, 10:28 AM

Updated to remove stray comment.

I think I would want some level of examination/analysis as to whether we want to do this with all of the vector-types, instead of just the ext_vector_type.

I think we should extend this to all vector types and matrix types. I only went with ext_vector_types to keep things simple initially to make sure the direction is acceptable first.

fhahn updated this revision to Diff 369136.Aug 27 2021, 10:30 AM
fhahn marked 2 inline comments as done.

Use ExprError() instead of ExprResult(true).

fhahn added inline comments.Aug 27 2021, 10:31 AM
clang/lib/Sema/SemaChecking.cpp
16544

Updated, thanks!

16554

I think we should extend this to all vector types and matrix types. I only went with ext_vector_types to keep things simple initially to make sure the direction is acceptable first.

16575

Ah sorry, that was a left-over from where I get the boilerplate code to start with. It's removed in the latest version.

Codewise I'm ok, but I don't feel comfortable making the direction decision. I'd like to see some level of 'state of things' on the vector types (like, under what conditions does GCC support this, and under what types??), but even then I'm not sure I'm the right one to approve this.

fhahn planned changes to this revision.Sep 17 2021, 2:03 AM

Codewise I'm ok, but I don't feel comfortable making the direction decision. I'd like to see some level of 'state of things' on the vector types (like, under what conditions does GCC support this, and under what types??), but even then I'm not sure I'm the right one to approve this.

Thanks! I'm preparing a proposal for cfe-dev at the moment. In addition to supporting element wise min/max/round & co I think we should also make sure the proposal also allows adding reduction versions in a consistent manner. I don't think that would be possible when overloading the existing builtins, so I am exploring a different direction at the moment.

Marking as changes planned for now.