This is an archive of the discontinued LLVM Phabricator instance.

[X86] Adding avx512_vpopcntdq feature set and its intrinsics
ClosedPublic

Authored by oren_ben_simhon on May 14 2017, 5:17 AM.

Details

Summary

AVX512_VPOPCNTDQ is a new feature set that Intel published here.

The patch represents the Clang side of the addition of six intrinsics for two new machine instructions (vpopcntd and vpopcntq).
It also includes the addition of the new feature set.

Diff Detail

Repository
rL LLVM

Event Timeline

RKSimon added a subscriber: RKSimon.
RKSimon added inline comments.
include/clang/Basic/BuiltinsX86.def
1105 ↗(On Diff #98920)

Would we be better off directly emitting Intrinsic::ctpop inside CGBuiltin.cpp?

AFAICT these builtins have no additional abilities compared to ctpop (other than masking which we can already handle).

Removed files properties

craig.topper edited edge metadata.May 14 2017, 8:58 AM

Looks like gcc already implements this https://www.mail-archive.com/gcc-patches@gcc.gnu.org/msg156421.html

Looks like their command line switch is -mavx512vpopcntdq and their preprocessor define is AVX512VPOPCNTDQ without an underscore. We should match for compatibility.

oren_ben_simhon marked an inline comment as done.

Implemented comments posted until 05/15 (Thanks Craig and Simon)

craig.topper edited subscribers, added: cfe-commits; removed: llvm-commits.May 15 2017, 2:24 PM
craig.topper added inline comments.May 15 2017, 2:32 PM
lib/CodeGen/CGBuiltin.cpp
7526 ↗(On Diff #99050)

I'm not sure what EmitScalarExpr does, but I got think its not right for a vector argument.

7527 ↗(On Diff #99050)

I think you can use Ops[0]->getType() and avoid the call to ConvertType. See the code for BI__builtin_ia32_vplzcntq_512_mask.

oren_ben_simhon marked an inline comment as done.

Implemented comments posted until 05/16 (Thanks Craig)

lib/CodeGen/CGBuiltin.cpp
7526 ↗(On Diff #99050)

Vector type is considered as scalar (single value) type from the emitter point of view.
See also: http://llvm.org/docs/LangRef.html#single-value-types

craig.topper added inline comments.May 18 2017, 9:36 AM
lib/CodeGen/CGBuiltin.cpp
7526 ↗(On Diff #99050)

Why can't we just use Ops[0] in place of X?

oren_ben_simhon marked an inline comment as done.

Implemented comments posted until 05/20 (Thanks Craig)

craig.topper added inline comments.May 21 2017, 12:58 AM
lib/CodeGen/CGBuiltin.cpp
7526 ↗(On Diff #99685)

Why did the call to ConvertType come back? This code can just be

llvm::Function *F = CGM.getIntrinsic(Intrinsic::ctpop, Ops[0]->getType());
return Builder.CreateCall(F, Ops);
lib/CodeGen/CGBuiltin.cpp
7526 ↗(On Diff #99685)

After checking it again, I reverted that change because AFAIK Ops[0] is the first argument and not the return type of the statement.

7527 ↗(On Diff #99050)

After checking it again, I am going to revert this change because AFAIK Ops[0] is the first argument and the return type.

lib/CodeGen/CGBuiltin.cpp
7526 ↗(On Diff #99685)

After checking it again, I reverted that change because AFAIK Ops[0] is the first argument and not the return type of the statement.

craig.topper accepted this revision.May 22 2017, 10:05 AM

LGTM

lib/CodeGen/CGBuiltin.cpp
7526 ↗(On Diff #99685)

That's true, but for this intrinsic they are required to be the same type. But its not a big deal.

This revision is now accepted and ready to land.May 22 2017, 10:05 AM
This revision was automatically updated to reflect the committed changes.