This is an archive of the discontinued LLVM Phabricator instance.

[x86] Fix bugs of some intrinsic functions in CLANG : _mm512_stream_ps, _mm512_stream_pd, _mm512_stream_si512
ClosedPublic

Authored by yubing on Aug 26 2019, 11:32 PM.

Diff Detail

Event Timeline

yubing created this revision.Aug 26 2019, 11:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 26 2019, 11:32 PM
yubing retitled this revision from [x86] Make some intrinsic functions in CLANG aligned with SPEC: _mm512_stream_ps, _mm512_stream_pd, _mm512_stream_si512 to [x86] Fix bugs of some intrinsic functions in CLANG : _mm512_stream_ps, _mm512_stream_pd, _mm512_stream_si512.Aug 26 2019, 11:34 PM
yubing edited the summary of this revision. (Show Details)
RKSimon added inline comments.Aug 27 2019, 5:21 AM
clang/test/CodeGen/avx512f-builtins.c
8570 ↗(On Diff #217315)

Would it make sense to keep the __m512* variants as well for test coverage?

yubing updated this revision to Diff 218290.Sep 2 2019, 12:03 AM
yubing marked an inline comment as done.

I think @RKSimon was asking to have tests with the original type and with the new void type. Just to verify that we accept code we used to accept and that we now accept void.

yubing updated this revision to Diff 218296.Sep 2 2019, 12:31 AM
RKSimon accepted this revision.Sep 2 2019, 5:48 AM

LGTM - thanks

This revision is now accepted and ready to land.Sep 2 2019, 5:48 AM
This revision was automatically updated to reflect the committed changes.

I think @RKSimon was asking to have tests with the original type and with the new void type. Just to verify that we accept code we used to accept and that we now accept void.

Sorry for ping this long long ago Phab... I just found we are missing alignment between these intrinsics' 512 variants and 128/256 ones. Does anyone remember the motivation here? The intrinsic guide also suffers such unaligning issue, but we can refine there, too if we could have a conclusion here. BTW gcc doesn't have such fix so they used aligned type* for these intrinsics.

Herald added a project: Restricted Project. · View Herald TranscriptAug 29 2023, 11:50 PM
Herald added a subscriber: StephenFan. · View Herald Transcript

I think @RKSimon was asking to have tests with the original type and with the new void type. Just to verify that we accept code we used to accept and that we now accept void.

Sorry for ping this long long ago Phab... I just found we are missing alignment between these intrinsics' 512 variants and 128/256 ones. Does anyone remember the motivation here? The intrinsic guide also suffers such unaligning issue, but we can refine there, too if we could have a conclusion here. BTW gcc doesn't have such fix so they used aligned type* for these intrinsics.

The tests (and the doxygen descriptions for sse2/avxavx2 headers) all have vector-alignment requirements - where did you find it missing?

FreddyYe added a comment.EditedAug 30 2023, 9:00 PM

I think @RKSimon was asking to have tests with the original type and with the new void type. Just to verify that we accept code we used to accept and that we now accept void.

Sorry for ping this long long ago Phab... I just found we are missing alignment between these intrinsics' 512 variants and 128/256 ones. Does anyone remember the motivation here? The intrinsic guide also suffers such unaligning issue, but we can refine there, too if we could have a conclusion here. BTW gcc doesn't have such fix so they used aligned type* for these intrinsics.

The tests (and the doxygen descriptions for sse2/avxavx2 headers) all have vector-alignment requirements - where did you find it missing?

The issue is 128/256 variants both used type* but 512 variants here used void*. I mean from the intrinsic level, not builtin.

FYI I raised a PR https://github.com/llvm/llvm-project/pull/66310 to align 128/256 variants to use void*. We can modify/inform intirnsic guide/gcc to align this change if the PR can be landed. Welcome comments there!