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
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
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
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,
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
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
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
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
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;
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,
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
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.
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
13 matches
Mail list logo