This is an archive of the discontinued LLVM Phabricator instance.

[X86] Fix some signedness errors in x86 headers
ClosedPublic

Authored by RKSimon on May 7 2022, 4:22 AM.

Details

Summary

Another step toward enabling -Wsystem-headers testing across all x86 headers

Fix a number of cases where the arg / return value signedness doesn't match the C/C++ intrinsic.

So far I've just added explicit casts as necessary, but we might want to address some of the mismatches directly:

e.g.

_mm512_srli_epi16(__m512i __A, unsigned int __B)
_mm512_mask_srli_epi16(__m512i __W, __mmask32 __U, __m512i __A, unsigned int __B)
_mm512_maskz_srli_epi16(__mmask32 __U, __m512i __A, int __B)  // <---- unsigned int?

Diff Detail

Event Timeline

RKSimon created this revision.May 7 2022, 4:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 7 2022, 4:22 AM
Herald added a subscriber: StephenFan. · View Herald Transcript
RKSimon requested review of this revision.May 7 2022, 4:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 7 2022, 4:22 AM
RKSimon updated this revision to Diff 427844.May 7 2022, 4:26 AM

Remove a couple of superfluous casts

RKSimon added inline comments.May 7 2022, 4:31 AM
clang/lib/Headers/cetintrin.h
26

@pengfei The Intel Intrisics guide has this taking a int ?

One question: is it better to change the define of builtins than explicit casts?

clang/lib/Headers/cetintrin.h
26

I think both make sense in some way. The instruction incsspq only takes the low 8-bit of a 64-bit register, but the operand is still 64-bit anyway.
I'm not sure which one we should change. Might be Clang given we specify unsigned int for _inc_ssp? Any special reason we use unsigned long long here @craig.topper ?

One question: is it better to change the define of builtins than explicit casts?

I can fix the signedness of the cet / rdseed / xbgen / tzcnt x86 builtins if there's a consensus.

What do you want to do about _mm512_maskz_srli_epi16 ? The Intel Intrinsic guide has the same mismatch.

Actually the ia32_tzcnt builtins should stay the way they are - other C/C++ intrinsics return unsigned so we'd still end up with adding explicit casts

RKSimon updated this revision to Diff 427860.May 7 2022, 7:05 AM

Fix signedness on 32-bit cet tests

RKSimon updated this revision to Diff 427863.May 7 2022, 7:28 AM

Move the exhaustive headers checks into x86-intrinsics-headers-clean.cpp as that's what its for

pengfei added a comment.EditedMay 7 2022, 7:44 AM

What do you want to do about _mm512_maskz_srli_epi16 ? The Intel Intrinsic guide has the same mismatch.

These intrinsics are interesting. The descriptions on Intrinsic guide are for immediate variant, but all compilers' implementations are register variant. What's more, the codegen from Clang and GCC don't seem correct according to the description of vpsrlw zmm0, zmm0, xmm1. They should do the same broadcast as ICC. https://godbolt.org/z/dcrqdEs8q

Back to the question, I think the type in Clang's intrinsics match with Intrinsic guide. They just not match each other. I guess it might be historical reasons, so let's keep them and using cast?

clang/lib/Headers/cetintrin.h
37

Unnecessary cast?

Actually the ia32_tzcnt builtins should stay the way they are - other C/C++ intrinsics return unsigned so we'd still end up with adding explicit casts

No problem, adding explicit casts look good to me.

These intrinsics are interesting. The descriptions on Intrinsic guide are for immediate variant, but all compilers' implementations are register variant. What's more, the codegen from Clang and GCC don't seem correct according to the description of vpsrlw zmm0, zmm0, xmm1. They should do the same broadcast as ICC. https://godbolt.org/z/dcrqdEs8q

After a second read, I found Clang and GCC's generation are also correct, I confused vpsrlw zmm0, zmm0, xmm1 with vpsrlw zmm0, zmm0, zmm1. Please ignore the noise.

RKSimon added inline comments.May 7 2022, 8:24 AM
clang/lib/Headers/cetintrin.h
37

yeah, we can just remove the (int)

RKSimon updated this revision to Diff 427868.May 7 2022, 8:25 AM

Remove superflouous cast from "_inc_ssp"

pengfei accepted this revision.May 7 2022, 5:41 PM

LGTM, thanks!

This revision is now accepted and ready to land.May 7 2022, 5:41 PM
This revision was landed with ongoing or failed builds.May 8 2022, 1:43 AM
This revision was automatically updated to reflect the committed changes.