This is an archive of the discontinued LLVM Phabricator instance.

[Builltins][X86] Provide implementations of __lzcnt16, __lzcnt, __lzcnt64 for MS compatibility. Remove declarations from intrin.h and implementations from lzcntintrin.h
ClosedPublic

Authored by craig.topper on Dec 13 2018, 2:49 PM.

Details

Summary

intrin.h had forward declarations for these and lzcntintrin.h had implementations that were only available with -mlzcnt or a -march that supported the lzcnt feature. But the 32-bit version was misnamed in lzcntrin.h

This patch provides an independent implementation for MS that doesn't rely on the X86 feature flag.

Fixes PR40014

Diff Detail

Event Timeline

craig.topper created this revision.Dec 13 2018, 2:49 PM
rnk added a comment.Dec 13 2018, 3:19 PM

This has the effect of removing __lzcnt* from non-MSVC targets using immintrin.h. Is that intended? It doesn't seem like these implementations were guarded by ifdef _MSC_VER.

Otherwise, yep, makes sense.

yeah I'm now realizing that gcc does implement lzcnt16 and lzcnt64. How do I make this work? I tried guarding them with _MSC_VER, but I got redefinition errors on a couple tests from that. Does -ms-extensions not define _MSC_VER?

rnk added a comment.Dec 13 2018, 3:36 PM

yeah I'm now realizing that gcc does implement lzcnt16 and lzcnt64. How do I make this work? I tried guarding them with _MSC_VER, but I got redefinition errors on a couple tests from that. Does -ms-extensions not define _MSC_VER?

I don't recall the details, but I suppose it's possible to get into that situation.

It'd be nice to have the builtin be the one true implementation, even with -fno-ms-extensions, but I don't see how to arrange things such that it's available only on x86 or any Windows target.

Replace the versions in lzcntintrin.h with macros that are only defined when _MSC_VER is not defined. This avoids a redefinition error when -ms-extensions is enabled which appears to not set _MSC_VER.

I forgot to mention previously that I changed 'LL' to 'W' in several builtin definitions to match the __int64 that MS documentation says they use.

rnk accepted this revision.Dec 13 2018, 4:04 PM

lgtm

The macro seems like a reasonable solution. It also helps keep the global namespace cleaner for non-windows users.

This revision is now accepted and ready to land.Dec 13 2018, 4:04 PM
This revision was automatically updated to reflect the committed changes.