[PATCH] D25264: Implement MS _BitScan intrinsics

majnemer accepted this revision. majnemer added a comment. This revision is now accepted and ready to land. This looks right to me. https://reviews.llvm.org/D25264 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D25264: Implement MS _BitScan intrinsics

agutowski updated this revision to Diff 74435. agutowski added a comment. rebase https://reviews.llvm.org/D25264 Files: include/clang/Basic/BuiltinsARM.def include/clang/Basic/BuiltinsX86.def include/clang/Basic/BuiltinsX86_64.def lib/Basic/Targets.cpp lib/CodeGen/CGBuiltin.cpp

[PATCH] D25264: Implement MS _BitScan intrinsics

majnemer added inline comments. > agutowski wrote in 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). It's needed for the call to CreatePHI to be in

[PATCH] D25264: Implement MS _BitScan intrinsics

agutowski added inline comments. > CGBuiltin.cpp:2665 > +BasicBlock *End = createBasicBlock("bitscan_end", this->CurFn); > +Builder.SetInsertPoint(End); > +PHINode *Result = Builder.CreatePHI(ResultType, 2, "bitscan_result"); Is this line needed? I took it from __builtin_fpclassify,

[PATCH] D25264: Implement MS _BitScan intrinsics

agutowski updated this revision to Diff 73685. agutowski added a comment. make _BitScan intrinsics compatible with Intel specification when the mask is zero https://reviews.llvm.org/D25264 Files: include/clang/Basic/BuiltinsARM.def include/clang/Basic/BuiltinsX86.def

[PATCH] D25264: Implement MS _BitScan intrinsics

majnemer added inline comments. > agutowski wrote in 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. Intel

[PATCH] D25264: Implement MS _BitScan intrinsics

rnk added inline comments. > CodeGenFunction.h:2964 > +private: > + enum class MSVCIntrin; > + Does this work on Linux? I thought you had to give it an explicit underlying type (enum class MSVCIntrin : unsigned;) to make that work. https://reviews.llvm.org/D25264

[PATCH] D25264: Implement MS _BitScan intrinsics

agutowski updated this revision to Diff 73668. agutowski added a comment. change enum in MSVC namespace to enum class MSVCIntrin in CodeGenFunction; cover all control paths https://reviews.llvm.org/D25264 Files: include/clang/Basic/BuiltinsARM.def include/clang/Basic/BuiltinsX86.def

[PATCH] D25264: Implement MS _BitScan intrinsics

agutowski added inline comments. > majnemer wrote in 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. I can see three options: (1) put the existing code inside an anonymous namespace;

[PATCH] D25264: Implement MS _BitScan intrinsics

rnk added a comment. Nice, probably ready to land with one revision. > majnemer wrote in 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. Let's also use a more specific name than MSVC,

[PATCH] D25264: Implement MS _BitScan intrinsics

agutowski added inline comments. > majnemer wrote in CGBuiltin.cpp: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. MSDN doesn't specify what should be put under the "Index" address when

[PATCH] D25264: Implement MS _BitScan intrinsics

majnemer added inline comments. > CGBuiltin.cpp:2640-2647 > +// Many of MSVC builtins are on both x64 and ARM; to avoid repeating code, we > +// handle them here. > +namespace MSVC { > + enum { > +_BitScanForward, > +_BitScanReverse > + }; This should be in an anonymous namespace.

[PATCH] D25264: Implement MS _BitScan intrinsics

agutowski created this revision. agutowski added reviewers: rnk, hans, thakis, majnemer. agutowski added a subscriber: cfe-commits. Herald added a subscriber: aemerson. _BitScan intrinsics (and some others, for example _Interlocked and _bittest) are supposed to work on both ARM and x86. This is