This is an archive of the discontinued LLVM Phabricator instance.

[Clang] Add __builtin_reduce_xor
ClosedPublic

Authored by junaire on Dec 7 2021, 2:56 AM.

Details

Summary

This patch implements __builtin_reduce_xor as specified in D111529.

Diff Detail

Event Timeline

junaire requested review of this revision.Dec 7 2021, 2:56 AM
junaire created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptDec 7 2021, 2:56 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
fhahn added inline comments.Dec 7 2021, 7:40 AM
clang/lib/Sema/SemaChecking.cpp
2219–2220

I think reduce_xor is only specified for integer types, so I think we need an extra check here?

junaire updated this revision to Diff 392785.Dec 8 2021, 8:07 AM
  • Add extra type check to the vector elements.
  • Add comment about type restriction.
  • polish tests a little bit.
fhahn added inline comments.Dec 8 2021, 11:51 AM
clang/lib/Sema/SemaChecking.cpp
2234

nit: the element type will be restricted to int, so maybe say something like only supports vectors of integers>

clang/test/Sema/builtins-reduction-math.c
50

vector of integers type sounds a bit off. Not sure if that's much better, but how about dropping the type from the end?

junaire updated this revision to Diff 393057.EditedDec 9 2021, 12:16 AM

Improve diagnostic message.

Fix failing CI by running:
arc diff git merge-base HEAD origin --update D115231

junaire updated this revision to Diff 393142.Dec 9 2021, 6:46 AM

Fix wrong code logic.

Any reason not to handle builtin_reduce_xor + builtin_reduce_or as well in the same patch since they are very similar?

Any reason not to handle builtin_reduce_xor + builtin_reduce_or as well in the same patch since they are very similar?

Hi, thanks for take a look at this.
Yes, you're right. The reason I split them is that I'm a beginner, so I want to send a "experimental patch" first, this will make review work a little bit easier if I mess something up. I think it's reasonable to add all similar builtins if currently nothing wrong about this one.

aaron.ballman added inline comments.Dec 15 2021, 4:40 AM
clang/lib/Sema/SemaChecking.cpp
2223

Please spell out the type here (same below).

2224

No need to repeat the type name here (same below).

junaire updated this revision to Diff 394563.Dec 15 2021, 7:31 AM

Explicitly speak out types for Arg, but don't repeat the vector type name.

aaron.ballman accepted this revision.Dec 15 2021, 8:11 AM

LGTM aside from a nit. Please wait a day or two before landing in case other reviewers have concerns they haven't had the chance to raise yet.

clang/include/clang/Sema/Sema.h
12787

If we're going to be renaming this, we should probably drop the Sema prefix from the name. (I'd note that we have a lot of functions prefixed with Sema that should be renamed, in case anyone really wants an NFC task that should be pretty trivial.)

This revision is now accepted and ready to land.Dec 15 2021, 8:11 AM
junaire updated this revision to Diff 394697.Dec 15 2021, 5:48 PM

Rename SemaBuiltinReduceMathPreCheck to PrepareBuiltinReduceMathOneArg

Any reason not to handle builtin_reduce_xor + builtin_reduce_or as well in the same patch since they are very similar?

Hi, thanks for take a look at this.
Yes, you're right. The reason I split them is that I'm a beginner, so I want to send a "experimental patch" first, this will make review work a little bit easier if I mess something up. I think it's reasonable to add all similar builtins if currently nothing wrong about this one.

That's fine - I'm happy to see these addressed as follow up patches - cheers!

fhahn accepted this revision.Dec 16 2021, 2:16 AM

LGTM, thanks

Thanks for accepting this patch! I would appreciate it if someone is willing to commit this for me since I don't have commit access ;D

You can use:
Jun Zhang
jun@junz.org

fhahn added a comment.Dec 21 2021, 7:11 AM

Thanks for accepting this patch! I would appreciate it if someone is willing to commit this for me since I don't have commit access ;D

You can use:
Jun Zhang
jun@junz.org

It looks like the patch doesn't apply cleanly on current main. Could you rebase the patch?

junaire updated this revision to Diff 395791.EditedDec 21 2021, 8:39 PM
  • rebase to main.
  • rename PrepareBuiltinReduceMathOneArg to PrepareBuiltinReduceMathOneArgCall.
This revision was landed with ongoing or failed builds.Dec 22 2021, 2:01 AM
Closed by commit rGb55ea2fbc0a0: [Clang] Add __builtin_reduce_xor (authored by junaire, committed by fhahn). · Explain Why
This revision was automatically updated to reflect the committed changes.