On Fri, Apr 12, 2013 at 1:22 AM, Richard Biener <richard.guent...@gmail.com> wrote: > On Thu, Apr 11, 2013 at 10:18 PM, Xinliang David Li <davi...@google.com> > wrote: >> Great that it is already covered. > > I expect that with more convoluted code you'll ICE eventually.
Yes, those are bugs that need to be fixed. > There is also > a bugreport about exactly this issue which should be referenced when a fix > is committed. there is a related thread from 2 years ago : http://gcc.gnu.org/ml/gcc-patches/2010-08/msg01417.html The patch was adding individual ISA macros eagerly which is not a good idea (as they are also used in user code). Sri's patch is certainly cleaner. > > Oh, and target maintainers disagree about if there is anything to fix and > what it would take to fix it ... > There are concerns about buggy function level target option handling, vector type lowering etc. It would be great if there are actual cases to demonstrate it. > Note that ICC happily expands the intrinsics to SSEX code even if > SSEX is not enabled. This seems handy, but may hide user errors. David > > Richard. > >> David >> >> On Thu, Apr 11, 2013 at 1:09 PM, Sriraman Tallam <tmsri...@google.com> wrote: >>> On Thu, Apr 11, 2013 at 1:05 PM, Sriraman Tallam <tmsri...@google.com> >>> wrote: >>>> On Thu, Apr 11, 2013 at 12:43 PM, Xinliang David Li <davi...@google.com> >>>> wrote: >>>>> What is the compile time impact for turning it on? Code not including >>>>> the intrinsic headers should not be affected too much. If the impact >>>>> is small, why not turning on this option by default -- which seems to >>>>> be the behavior of ICC. >>>> >>>> I will get back with data on this. >>>> >>>>> >>>>> With this option, all functions without the appropriate target options >>>>> will be allowed to reference not supported builtins, without warnings >>>>> or errors. Is it possible to delay the check till the builtin >>>>> expansion time? >>>> >>>> Right now, an error is generated if a function accesses an unsupported >>>> builtin. Since the intrinsic functions are marked inline and call some >>>> target builtin, this will always be caught. >>> >>> To be clear, same example without the target attribute and with >>> -mgenerate-builtins results in an error: >>> >>> #include <smmintrin.h> >>> >>> __m128i foo(__m128i *V) >>> { >>> return _mm_stream_load_si128(V); >>> } >>> >>> $ g++ test.cc -mgenerate-builtins >>> >>> smmintrin.h:582:59: error: ‘__builtin_ia32_movntdqa’ needs isa option >>> -m32 -msse4.1 >>> return (__m128i) __builtin_ia32_movntdqa ((__v2di *) __X); >>> >>> >>>> >>>> Sri >>>> >>>>> >>>>> thanks, >>>>> >>>>> David >>>>> >>>>> On Thu, Apr 11, 2013 at 12:05 PM, Sriraman Tallam <tmsri...@google.com> >>>>> wrote: >>>>>> Hi, >>>>>> >>>>>> *mmintrin headers does not work with function specific opts. >>>>>> >>>>>> Example 1: >>>>>> >>>>>> >>>>>> #include <smmintrin.h> >>>>>> >>>>>> __attribute__((target("sse4.1"))) >>>>>> __m128i foo(__m128i *V) >>>>>> { >>>>>> return _mm_stream_load_si128(V); >>>>>> } >>>>>> >>>>>> >>>>>> $ g++ test.cc >>>>>> smmintrin.h:31:3: error: #error "SSE4.1 instruction set not enabled" >>>>>> # error "SSE4.1 instruction set not enabled" >>>>>> >>>>>> This error happens even though foo is marked "sse4.1" >>>>>> >>>>>> There are multiple issues at play here. One, the headers are guarded >>>>>> by macros that are switched on only when the target specific options, >>>>>> like -msse4.1 in this case, are present in the command line. Also, the >>>>>> target specific builtins, like __builtin_ia32_movntdqa called by >>>>>> _mm_stream_load_si128, are exposed only in the presence of the >>>>>> appropriate target ISA option. >>>>>> >>>>>> >>>>>> I have attached a patch that fixes this. I have added an option >>>>>> "-mgenerate-builtins" that will do two things. It will define a macro >>>>>> "__ALL_ISA__" which will expose the *intrin.h functions. It will also >>>>>> expose all the target specific builtins. -mgenerate-builtins will not >>>>>> affect code generation. >>>>>> >>>>>> This feature will greatly benefit the function multiversioning usability >>>>>> too. >>>>>> >>>>>> Comments? >>>>>> >>>>>> Thanks >>>>>> Sri