This is an archive of the discontinued LLVM Phabricator instance.

[X86] Enable constexpr on POPCNT intrinsics (PR31446)
ClosedPublic

Authored by RKSimon on Aug 19 2020, 9:58 AM.

Details

Summary

This is a RFC first step patch to enable constexpr support and testing to a large number of x86 intrinsics.

All I've done here is provide a DEFAULT_FN_ATTRS_CONSTEXPR variant to our existing DEFAULT_FN_ATTRS tag approach that adds constexpr on c++ builds. The clang cuda headers do something similar.

I've started with POPCNT mainly as its tiny and are wrappers to generic __builtin_* intrinsics which already act as constexpr.

Diff Detail

Event Timeline

RKSimon created this revision.Aug 19 2020, 9:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 19 2020, 9:58 AM
RKSimon requested review of this revision.Aug 19 2020, 9:58 AM
craig.topper added inline comments.Aug 19 2020, 10:05 AM
clang/lib/Headers/popcntintrin.h
16

Do we need to check for C++11?

RKSimon added inline comments.Aug 19 2020, 10:11 AM
clang/lib/Headers/popcntintrin.h
16

I don't actually know - its trivial to add a __cplusplus >= 201103L check but I don't know what we are expected to support in compiler headers like these.

arsenm added a subscriber: arsenm.Aug 19 2020, 10:14 AM
arsenm added inline comments.
clang/test/CodeGen/popcnt-builtins.cpp
4

Missing required register target?

RKSimon updated this revision to Diff 286601.Aug 19 2020, 10:15 AM

add c++11 limiter

craig.topper added inline comments.Aug 19 2020, 10:19 AM
clang/test/CodeGen/popcnt-builtins.cpp
4

Do we need a registered target if we're stopping at IR generation?

RKSimon added inline comments.Aug 19 2020, 10:20 AM
clang/test/CodeGen/popcnt-builtins.cpp
4

sure, oddly we don't appear to do this for most of the x86 builtins codegen tests - I'll see if I can cleanup those soon

RKSimon updated this revision to Diff 286605.Aug 19 2020, 10:23 AM

Require x86-registered-target

RKSimon added inline comments.Aug 19 2020, 10:35 AM
clang/test/CodeGen/popcnt-builtins.cpp
4

Not really - I'm not sure if its good practice or not, some targets seem to do this but x86 not so much.

arsenm added inline comments.Aug 19 2020, 11:10 AM
clang/test/CodeGen/popcnt-builtins.cpp
4

There's a vague desire to be able to omit the target intrinsics if the target isn't built, which would imply eventually making the clang parts conditionally compiled

RKSimon added inline comments.Aug 19 2020, 12:10 PM
clang/test/CodeGen/popcnt-builtins.cpp
4

I'm blanking on this (loooooong week) - but isn't there a way to tell the driver to treat this as a c program? Then we can either add run lines that test it as 'c' in the (renamed) cpp file or 'c++11' in the original c file - I don't mind which.

erichkeane added inline comments.Aug 19 2020, 12:11 PM
clang/test/CodeGen/popcnt-builtins.cpp
4

-x c does it.

RKSimon updated this revision to Diff 286634.Aug 19 2020, 12:40 PM
RKSimon edited the summary of this revision. (Show Details)

Added -x c++ RUN lines to the existing popcnt-builtins.c file - thanks @erichkeane

I think that's everything - I'd prefer to handle the registered target fixes in separate commits.

RKSimon updated this revision to Diff 286835.Aug 20 2020, 9:00 AM

Added release note entry - this will need to revised as we add more intrinsics, so is just an initial placeholder.

RKSimon updated this revision to Diff 286837.Aug 20 2020, 9:01 AM
craig.topper added a comment.EditedAug 20 2020, 9:37 AM

Should we test the intrinsics in a constexpr context? Something like https://godbolt.org/z/6o5zPn should work with the intrinsic instead of the builtin

RKSimon updated this revision to Diff 286861.Aug 20 2020, 10:49 AM

Added constexpr tests - I went for the -ve array size approach as thats the approach in clang\test\Sema\constant-builtins-2.c etc.

This revision is now accepted and ready to land.Aug 20 2020, 12:22 PM
This revision was landed with ongoing or failed builds.Aug 20 2020, 1:39 PM
This revision was automatically updated to reflect the committed changes.