Page MenuHomePhabricator

[X86] VP2INTERSECT clang
ClosedPublic

Authored by xiangzhangllvm on May 23 2019, 11:55 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMay 23 2019, 11:55 PM
craig.topper retitled this revision from VP2INTERSECT clang to [X86] VP2INTERSECT clang.May 24 2019, 12:11 AM
craig.topper added reviewers: spatel, RKSimon.
craig.topper added inline comments.
lib/Basic/Targets/X86.cpp
518 ↗(On Diff #201130)

This patch needs to be rebased. I modified the formatting here earlier to fix the weird indentation.

craig.topper added inline comments.May 27 2019, 10:11 PM
lib/Headers/avx512vlvp2intersectintrin.h
39 ↗(On Diff #201611)

Can you add doxygen comments for the new intrinsics? @RKSimon has been asking for it on other reviews. I forgot to say something in our internal review.

xiangzhangllvm marked an inline comment as done.May 27 2019, 10:39 PM
xiangzhangllvm added inline comments.
lib/Headers/avx512vlvp2intersectintrin.h
39 ↗(On Diff #201611)

OK! But I really find many ntrinsicxxx.h have no doxygen comments, Is it like this format: ?

/// Rounds up each element of the 128-bit vector of [4 x float] to an
///    integer and returns the rounded values in a 128-bit vector of
///    [4 x float].
///
/// \headerfile <x86intrin.h>
///
/// \code
/// __m128 _mm_ceil_ps(__m128 X);
/// \endcode
///
/// This intrinsic corresponds to the <c> VROUNDPS / ROUNDPS </c> instruction.
///
/// \param X
///    A 128-bit vector of [4 x float] values to be rounded up.
/// \returns A 128-bit vector of [4 x float] containing the rounded values.
#define _mm_ceil_ps(X)       _mm_round_ps((X), _MM_FROUND_CEIL)
craig.topper added inline comments.May 27 2019, 10:43 PM
lib/Headers/avx512vlvp2intersectintrin.h
39 ↗(On Diff #201611)

Yeah that looks right. Someone was kind enough to provide comments for some of the header files a few years ago, but it wasn't complete. I know avx512* especially are missing comments.

xiangzhangllvm marked 2 inline comments as done.May 28 2019, 12:01 AM
RKSimon added inline comments.May 28 2019, 3:45 AM
lib/Headers/avx512vlvp2intersectintrin.h
39 ↗(On Diff #201611)

The hope is that these will get added as time goes on - usually it just requires a copy + paste of existing comments and tweak for ymm/zmm/whatever.

New intrinsics should always include documentation since the person adding it is the most likely to be able to correct describe it.

xiangzhangllvm marked an inline comment as done.May 28 2019, 4:41 AM
xiangzhangllvm added inline comments.
lib/Headers/avx512vlvp2intersectintrin.h
39 ↗(On Diff #201611)

Yes, That make sense!

Hi Dear friends, could you help merge this patch? Thank you very much!

craig.topper added inline comments.May 30 2019, 7:09 PM
lib/Headers/avx512vlvp2intersectintrin.h
39 ↗(On Diff #202350)

This doesn't really say anything about what's in the mask registers. I don' think any of the MODRM encoding details are relevant here. I think a better description would be what's in the table in the ISE manual. "Store, in an even/odd pair of mask registers,
the indicators of the locations of value
matches between dwords in
xmm3/m128/m32bcst and xmm2."

46 ↗(On Diff #202350)

Drop the "AVX512-". Add the proper D/Q suffix. This should be the instruction mnemonic

53 ↗(On Diff #202350)

ponit?

xiangzhangllvm marked 3 inline comments as done.

Done, Thank you very much!

This revision is now accepted and ready to land.May 30 2019, 8:36 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 30 2019, 11:07 PM