This is an archive of the discontinued LLVM Phabricator instance.

Implement MS _BitScan intrinsics
ClosedPublic

Authored by agutowski on Oct 4 2016, 7:24 PM.

Details

Summary

_BitScan intrinsics (and some others, for example _Interlocked and _bittest) are supposed to work on both ARM and x86. This is an attempt to isolate them, avoiding repeating their code or writing separate function for each builtin.

Event Timeline

agutowski updated this revision to Diff 73588.Oct 4 2016, 7:24 PM
agutowski retitled this revision from to Implement MS _BitScan intrinsics.
agutowski updated this object.
agutowski added reviewers: rnk, hans, thakis, majnemer.
agutowski added a subscriber: cfe-commits.
majnemer added inline comments.Oct 4 2016, 8:51 PM
lib/CodeGen/CGBuiltin.cpp
2640–2647

This should be in an anonymous namespace. Also, consider using an enum class instead of an enum nested inside a namespace.

2656–2684

Does this do the right thing if the arg is zero? I think it would if you gave the call to the intrinsic an operand of false instead of true.

agutowski added inline comments.Oct 5 2016, 9:02 AM
lib/CodeGen/CGBuiltin.cpp
2656–2684

MSDN doesn't specify what should be put under the "Index" address when the argument is zero; as I checked, VS2015 with optimizations puts undefined value there, and I hope that's what I'm doing here.

rnk edited edge metadata.Oct 5 2016, 9:52 AM

Nice, probably ready to land with one revision.

lib/CodeGen/CGBuiltin.cpp
2640–2647

Let's also use a more specific name than MSVC, maybe MSVCIntrin or something.

agutowski added inline comments.Oct 5 2016, 9:55 AM
lib/CodeGen/CGBuiltin.cpp
2640–2647

I can see three options:
(1) put the existing code inside an anonymous namespace;
(2) create a private enum class in CodeGenFunction;
(3) pull EmitMSVCBuiltinExpr outside of CodeGenFunction and take CGF object as an argument (and then make enum class inside an anonymous namespace);
I don't really like any of them. Enum class sounds nice as I can imagine someone trying to pass the global builtin ID (like X86::BI_BitScanForward) instead of the one from MSVCIntrin namespace (although it should fail on any test; still, it would compile without enum class). But builtins use CGF methods all of the time, so pulling it out will make the code of every bulitin uglier. So I guess I'll try to go with the private enum class inside the CodeGenFunction, but if you have any better ideas, I'm eager to listen.

agutowski updated this revision to Diff 73668.Oct 5 2016, 10:18 AM
agutowski edited edge metadata.

change enum in MSVC namespace to enum class MSVCIntrin in CodeGenFunction; cover all control paths

rnk added inline comments.Oct 5 2016, 10:43 AM
lib/CodeGen/CodeGenFunction.h
2964

Does this work on Linux? I thought you had to give it an explicit underlying type (enum class MSVCIntrin : unsigned;) to make that work.

agutowski added inline comments.Oct 5 2016, 10:49 AM
lib/CodeGen/CodeGenFunction.h
2964

Yes, it seems to work on Linux too. According to the cppreference.com, it "declares a scoped enumeration type whose underlying type is int".

majnemer added inline comments.Oct 5 2016, 11:20 AM
lib/CodeGen/CGBuiltin.cpp
2656–2684

Intel (https://software.intel.com/en-us/node/523362) says the following:
Sets *p to the bit index of the least significant set bit of b or leaves it unchanged if b is zero. The function returns a non-zero result when b is non-zero and returns zero when b is zero.

This seems to mesh with this Mozilla bug report: https://bugzilla.mozilla.org/show_bug.cgi?id=1182370

agutowski updated this revision to Diff 73685.Oct 5 2016, 1:03 PM

make _BitScan intrinsics compatible with Intel specification when the mask is zero

agutowski added inline comments.Oct 5 2016, 1:07 PM
lib/CodeGen/CGBuiltin.cpp
2665

Is this line needed? I took it from __builtin_fpclassify, but I don't know what could be its purpose (it's repeated below, where the "bitscan_end" block really starts).

majnemer added inline comments.Oct 5 2016, 2:44 PM
lib/CodeGen/CGBuiltin.cpp
2665

It's needed for the call to CreatePHI to be in the correct basic block.

RKSimon added a subscriber: RKSimon.Oct 6 2016, 2:55 PM
agutowski marked an inline comment as done.Oct 11 2016, 10:48 AM
majnemer accepted this revision.Oct 12 2016, 2:50 PM
majnemer edited edge metadata.

This looks right to me.

This revision is now accepted and ready to land.Oct 12 2016, 2:50 PM
agutowski closed this revision.Oct 12 2016, 3:10 PM