This is an archive of the discontinued LLVM Phabricator instance.

[x86] invpcid intrinsic
ClosedPublic

Authored by GBuella on May 21 2018, 7:55 AM.

Details

Summary

An intrinsic for an old instruction, as described in the Intel SDM.

Diff Detail

Event Timeline

GBuella created this revision.May 21 2018, 7:55 AM
craig.topper added inline comments.
lib/Headers/cpuid.h
158

this should be below ENH_MOVSB to keep the bits in order

lib/Headers/intrin.h
196 ↗(On Diff #147784)

@rnk, what's the right thing to do here?

rnk added a comment.May 21 2018, 9:50 AM

Why do we need a feature flag for this in the first place? The MSVC model for most "instruction" intrinsics is that the exact instruction is emitted regardless of the feature enabled. The target attribute seems like it would get in the way of that.

lib/Headers/intrin.h
196 ↗(On Diff #147784)

What problems does this redeclaration cause?

In D47142#1106664, @rnk wrote:

Why do we need a feature flag for this in the first place? The MSVC model for most "instruction" intrinsics is that the exact instruction is emitted regardless of the feature enabled. The target attribute seems like it would get in the way of that.

I'm fine with getting rid of the feature flag, here, and then probably also for other instructions in similar situations (pconfig, wbnoinvd, clwb, etc...).
But as long as we keep those other feature flags, I guess we should just be consistent.

lib/Headers/intrin.h
196 ↗(On Diff #147784)

Now that I think about it, none.

GBuella added inline comments.May 21 2018, 10:13 AM
lib/Headers/intrin.h
196 ↗(On Diff #147784)

Also, I think this could be added as TARGET_HEADER_BUILTIN..."intrin.h", ALL_MS_LANGUAGES into BuiltinsX86.def
And removed from here.
Right?

rnk added inline comments.May 21 2018, 10:15 AM
lib/Headers/intrin.h
196 ↗(On Diff #147784)

It could be, but then you'd need to implement it as a builtin instead of having invpcidintrin.h, right?

GBuella added inline comments.May 21 2018, 12:01 PM
lib/Headers/intrin.h
196 ↗(On Diff #147784)

Meanwhile, I just realized, we add the !defined(_MSC_VER) condition around the inclusion of all such header files, as I did with invpcidintrin.h.
Does clang always define _MSC_VER on Windows? I mean, always in cases where one would want to include the MSVC likes`intrin.h`?
In that case, these are completely independent things. Also, I could just add both builtins _invpcid with the ALL_MS_LANGUAGES thing, and the __builtin_ia32_invpcid with the usual clang/GCC convention of adding separate feature flags (both would be lowered to the same LLVM intrinsic).
BTW, if we stop adding feature flags for such intrinsics, we probably have to convince GCC to do the same, as we love that compatibility.

GBuella updated this revision to Diff 148011.May 22 2018, 8:21 AM
GBuella updated this revision to Diff 148341.May 23 2018, 11:52 PM

Relocated header inclusion from x86intrin.h -> immintrin.h
I gave up on the MSVC intrinsic idea, it wouldn't work in a way compatible with MSVC as long as LLVM requires the feature to be enabled anyways. If once we decide to remove such feature flags for such system programming instructions, we can get back this.

LGTM, if you fix the ordering in cpuid.h.

craig.topper accepted this revision.May 24 2018, 1:01 PM
This revision is now accepted and ready to land.May 24 2018, 1:01 PM
This revision was automatically updated to reflect the committed changes.