[PATCH] D33616: [MS] Fix _bittest* intrinsics for values bigger than 31

2017-05-31 Thread Richard Smith - zygoloid via Phabricator via cfe-commits
rsmith added a comment. My feeling is that there's not much value in adding an LLVM intrinsic for something that can be expressed directly in a handful of IR instructions; those instructions seem like the way to express this that would allow the most optimization potential, and the backend shou

[PATCH] D33616: [MS] Fix _bittest* intrinsics for values bigger than 31

2017-05-31 Thread Reid Kleckner via Phabricator via cfe-commits
rnk added a comment. Darn, I think Richard is right, the signed div/mod doesn't do the right thing for negative values. Honestly I need to rig up a test case for this, and then I'll come back to it. What do you folks think is best: - Add an LLVM intrinsic for this and use it - Use inline assem

[PATCH] D33616: [MS] Fix _bittest* intrinsics for values bigger than 31

2017-05-30 Thread David Majnemer via Phabricator via cfe-commits
majnemer added inline comments. Comment at: clang/lib/Headers/intrin.h:345-347 + _BitBase += (_BitPos / 32); + _BitPos %= 32; return (*_BitBase >> _BitPos) & 1; `_bittest` seems to expand to `(((unsigned char const *)_BitBase)[_BitPos >> 3] >> (_BitPos & 7

[PATCH] D33616: [MS] Fix _bittest* intrinsics for values bigger than 31

2017-05-30 Thread Richard Smith - zygoloid via Phabricator via cfe-commits
rsmith added a comment. In https://reviews.llvm.org/D33616#768287, @hans wrote: > From looking in the Intel manual (Table 3-2, in 3.1.1.9 about > Bit(BitBase,BitOffset)) it does sound like the bit offset can be negative > *shudder*, so I suppose this is necessary and explains why the type is si

[PATCH] D33616: [MS] Fix _bittest* intrinsics for values bigger than 31

2017-05-30 Thread David Majnemer via Phabricator via cfe-commits
majnemer accepted this revision. majnemer added a comment. LGTM https://reviews.llvm.org/D33616 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33616: [MS] Fix _bittest* intrinsics for values bigger than 31

2017-05-30 Thread David Majnemer via Phabricator via cfe-commits
majnemer added a comment. In https://reviews.llvm.org/D33616#768287, @hans wrote: > From looking in the Intel manual (Table 3-2, in 3.1.1.9 about > Bit(BitBase,BitOffset)) it does sound like the bit offset can be negative > *shudder*, so I suppose this is necessary and explains why the type is

[PATCH] D33616: [MS] Fix _bittest* intrinsics for values bigger than 31

2017-05-30 Thread Hans Wennborg via Phabricator via cfe-commits
hans added a comment. From looking in the Intel manual (Table 3-2, in 3.1.1.9 about Bit(BitBase,BitOffset)) it does sound like the bit offset can be negative *shudder*, so I suppose this is necessary and explains why the type is signed in the first place? Hopefully most of these will be inlined

[PATCH] D33616: [MS] Fix _bittest* intrinsics for values bigger than 31

2017-05-30 Thread David Majnemer via Phabricator via cfe-commits
majnemer added a comment. Do you really want to be doing signed division here? Because it is signed, it won't turn into the simple bitshifts and masks that one might expect. https://reviews.llvm.org/D33616 ___ cfe-commits mailing list cfe-commits@l

[PATCH] D33616: [MS] Fix _bittest* intrinsics for values bigger than 31

2017-05-30 Thread Hans Wennborg via Phabricator via cfe-commits
hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. lgtm https://reviews.llvm.org/D33616 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commi

[PATCH] D33616: [MS] Fix _bittest* intrinsics for values bigger than 31

2017-05-26 Thread Reid Kleckner via Phabricator via cfe-commits
rnk created this revision. These intrinsics are supposed to select to BT, BTS, etc instructions. Those instructions actually perform a bitwise array indexing memory operation that LLVM doesn't currently expose. This change implements the shifting and array indexing in plain C. Fixes PR33188 If