This is an archive of the discontinued LLVM Phabricator instance.

Remove duplicate type definitions from f16cintrin.h
AbandonedPublic

Authored by jtsoftware on May 7 2015, 6:02 AM.

Details

Summary

These definitions can be obtained from avxintrin.h. Modularize flags them as duplicates.

Diff Detail

Event Timeline

jtsoftware updated this revision to Diff 25160.May 7 2015, 6:02 AM
jtsoftware retitled this revision from to Remove duplicate type definitions from f16cintrin.h.
jtsoftware updated this object.
jtsoftware edited the test plan for this revision. (Show Details)
jtsoftware added reviewers: gaoyunzhong, ygao.
jtsoftware added a subscriber: Unknown Object (MLST).
ygao accepted this revision.May 11 2015, 12:38 PM
ygao edited edge metadata.

LGTM. This is compatible with GCC headers.
By the way, this review did not appear on the mailing list for some reason,

This revision is now accepted and ready to land.May 11 2015, 12:38 PM
silvas requested changes to this revision.May 11 2015, 6:04 PM
silvas added a reviewer: silvas.
silvas added a subscriber: silvas.

Please include the header where those types are actually defined. We're trying to be modular here.

This revision now requires changes to proceed.May 11 2015, 6:04 PM

Please include the header where those types are actually defined. We're trying to be modular here.

I suspect the correct fix there is to #include <f16cintrin.h> from immintrin.h instead of x86intrin.h; that's what gcc does.

Looks like there's more duplication to get rid of in this patch.

lib/Headers/avxintrin.h:typedef float __v8sf __attribute__ ((__vector_size__ (32)));
lib/Headers/f16cintrin.h:typedef float __v8sf __attribute__ ((__vector_size__ (32)));

lib/Headers/avxintrin.h:typedef float __m256 __attribute__ ((__vector_size__ (32)));
lib/Headers/f16cintrin.h:typedef float __m256 __attribute__ ((__vector_size__ (32)));

What is the right dependency order between these headers?

Gah, nevermind. Brain fart.

If we want individual headers to be independent, we probably need a new header called something like. _intrinsic_types.h, as there are a few definitions like m256, m256d, and __m256i scattered among multiple headers. But actually, it seems to be the case that headers like f16cintrin.h are private headers under x86intrin.h, as indicated in the #error directive (#error "Never use <f16cintrin.h> directly; include <x86intrin.h> instead."), hence the need to be independent could be considered relaxed. If gnu is like this, it might be a good reason not to stray too far.

What do you think?

jtsoftware abandoned this revision.Mar 21 2016, 1:11 PM

It looks like this has already been addressed.