Page MenuHomePhabricator

[Clang] Implement the rest of __builtin_elementwise_* functions.
ClosedPublic

Authored by junaire on Dec 9 2021, 12:05 AM.

Details

Summary

The patch implement the rest of __builtin_elementwise_* functions
specified in D111529, including:

  • __builtin_elementwise_floor
  • __builtin_elementwise_roundeven
  • __builtin_elementwise_trunc

Signed-off-by: Jun <jun@junz.org>

Diff Detail

Event Timeline

junaire requested review of this revision.Dec 9 2021, 12:05 AM
junaire created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptDec 9 2021, 12:05 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
aaron.ballman added inline comments.Dec 15 2021, 4:35 AM
clang/include/clang/Basic/Builtins.def
651

The RFC does not mention this builtin. It specifies __builtin_elementwise_rint and __builtin_elementwise_round. Is there a reason why this uses roundeven instead and should there be other rounding options provided at the same time?

clang/lib/CodeGen/CGBuiltin.cpp
3139–3155

It's starting to seem like we should have a helper function that takes the intrinsic to create and the name to give the operation; WDYT?

The RFC does not mention this builtin. It specifies __builtin_elementwise_rint and __builtin_elementwise_round. Is there a reason why this uses roundeven instead and should there be other rounding options provided at the same time?

Well, I think there may be a misunderstanding. I referenced: https://clang.llvm.org/docs/LanguageExtensions.html#vector-builtins
There is this builtin:

T __builtin_elementwise_roundeven(T x)

But I just found in the RFC(https://lists.llvm.org/pipermail/cfe-dev/2021-September/068999.html), we have:

• __builtin_elementwise_rint
• __builtin_elementwise_round

It looks confusing, I would like to review the RFC again.

junaire updated this revision to Diff 395322.Dec 19 2021, 5:51 AM

Add a helper function to resue code. I'm not good at naming functions,
so I just oveloaded emitUnaryBuiltin.

junaire updated this revision to Diff 395323.Dec 19 2021, 5:53 AM

Format the patch.

Hi, @aaron.ballman I'm sorry for not updating the patch in time because I'm preparing for my school final exam :-(
One thing I want to mention is that __builtin_elementwise_roundeven is actually been added in the RFC during the code review. You can find it in D111529.

"Prevailing rounding mode" is not super-useful, other than as a spelling for round-to-nearest-ties-to-even (IEEE 754 default rounding). Outside of a FENV_ACCESS ON context, there's not even really a notion of "prevailing rounding mode" to appeal to. I assume the intent is for this to lower to e.g. x86 ROUND* with the dynamic rounding-mode immediate.

I would recommend adding __builtin_elementwise_roundeven(T x) instead, which would statically bind IEEE default rounding (following TS 18661-1 naming) without having to appeal to prevailing rounding mode, and can still lower to ROUND* on x86 outside of FENV_ACCESS ON contexts, which is the norm for vector code (and FRINTN unconditionally on armv8). I think we can punt on rint/nearbyint for now, and add them in the future if there's a need.

Hi, @aaron.ballman I'm sorry for not updating the patch in time because I'm preparing for my school final exam :-(
One thing I want to mention is that __builtin_elementwise_roundeven is actually been added in the RFC during the code review. You can find it in D111529.

"Prevailing rounding mode" is not super-useful, other than as a spelling for round-to-nearest-ties-to-even (IEEE 754 default rounding). Outside of a FENV_ACCESS ON context, there's not even really a notion of "prevailing rounding mode" to appeal to. I assume the intent is for this to lower to e.g. x86 ROUND* with the dynamic rounding-mode immediate.

I would recommend adding __builtin_elementwise_roundeven(T x) instead, which would statically bind IEEE default rounding (following TS 18661-1 naming) without having to appeal to prevailing rounding mode, and can still lower to ROUND* on x86 outside of FENV_ACCESS ON contexts, which is the norm for vector code (and FRINTN unconditionally on armv8). I think we can punt on rint/nearbyint for now, and add them in the future if there's a need.

Yes, this was a change from the RFC after some feedback to the patch.

clang/lib/CodeGen/CGBuiltin.cpp
3139–3155

Sounds good! Might be good to split the emitUnaryBuiltin changes off into a separate change?

Sounds good! Might be good to split the emitUnaryBuiltin changes off into a separate change?

Thanks for your suggestion! I just sent another patch: D116161

fhahn added a comment.Jan 4 2022, 3:49 AM

This now needs a rebase after landing 5c57e6aa5777, then it should be good to go.

junaire updated this revision to Diff 397260.Jan 4 2022, 4:11 AM

rebase to main.

This now needs a rebase after landing 5c57e6aa5777, then it should be good to go.

Well, I just found that you seem to have committed under the wrong name...
My name is Jun Zhang, not Jun Zhan...
But I think it's fine. :)

fhahn added a comment.Jan 4 2022, 5:47 AM

This now needs a rebase after landing 5c57e6aa5777, then it should be good to go.

Well, I just found that you seem to have committed under the wrong name...
My name is Jun Zhang, not Jun Zhan...
But I think it's fine. :)

I am sorry for the typo! I reverted & recommitted the patch with the hopefully correct name now :)

fhahn accepted this revision.Jan 4 2022, 6:02 AM

LGTM, thanks!

This revision is now accepted and ready to land.Jan 4 2022, 6:02 AM

LGTM, thanks!

May I ask you to help me to land this patch? I plan to request commit access after this one. Thanks a lot! ;D

This revision was landed with ongoing or failed builds.Jan 7 2022, 7:12 AM
This revision was automatically updated to reflect the committed changes.