+HJ
On Tue, Apr 16, 2013 at 1:54 PM, Sriraman Tallam <tmsri...@google.com> wrote:
> Hi,
>
> I have attached an updated patch that addresses all the comments raised.
>
> On Fri, Apr 12, 2013 at 1:58 AM, Jakub Jelinek <ja...@redhat.com> wrote:
>> On Thu, Apr 11, 2013 at 12:05:41PM -0700, Sriraman Tallam wrote:
>>> 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.
>>
>> 1) this shouldn't be an option, either it can be made to work reliably,
>> then it should be done always, or it can't, then it shouldn't be done
>
> Ok, it is on by default now. There is a way to turn it off, with
> -mno-generate-builtins.
>
>> 2) have you verified that if you always generate all builtins, that the
>> builtins not supported by the ISA selected from the command line are
>> created with the right vector modes?
>
> This issue does not arise. When the target builtin is expanded, it is
> checked if the ISA support is there, either via function specific
> target opts or global target opts. If not, an error is raised. Test
> case added for this, please see intrinsic_4.c in patch.
>
>> 3) the *intrin.h headers in the case where the guarding macro isn't defined
>> should be surrounded by something like
>> #ifndef __FMA4__
>> #pragma GCC push options
>> #pragma GCC target("fma4")
>> #endif
>> ...
>> #ifndef __FMA4__
>> #pragma GCC pop options
>> #endif
>> so that everything that is in the headers is compiled with the ISA
>> in question
>
> I do not think this should be done because it will break the inlining
> ability of the header function and cause issues if the caller does not
> specify the required ISA. The fact that the header functions are
> marked extern __inline, with gnu_inline guarantees that a body will
> not be generated and they will be inlined. If the caller does not
> have the required ISA, appropriate errors will be raised. Test cases
> added, see intrinsics_1.c, intrinsics_2.c
>
>> 4) what happens if you use the various vector types typedefed in the
>> *intrin.h headers in code that doesn't support those ISAs? As TYPE_MODE
>> for VECTOR_TYPE is a function call, perhaps it will just be handled as
>> generic BLKmode vectors, which is desirable I think
>
> I checked some tests here. With -mno-sse for instance, vector types
> are not permitted in function arguments and return values and gcc
> raises a warning/error in each case. With return values, gcc always
> gives an error if a SSE register is required in a return value. I
> even fixed this message to not do it for functions marked as extern
> inline, with "gnu_inline" keyword as a body for them will not be
> generated.
>
>
>> 5) what happens if you use a target builtin in a function not supporting
>> the corresponding ISA, do you get proper error explaining what you are
>> doing wrong?
>
> Yes, please sse intrinsic_4.c test in patch.
>
>> 6) what happens if you use some intrinsics in a function not supporting
>> the corresponding ISA? Dunno if the inliner chooses not to inline it
>> and error out because it is always_inline, or what exactly will happen
>> then
>
> Same deal here. The intrinsic function will, guaranteed, to be inlined
> into the caller which will be a corresponding builtin call. That
> builtin call will trigger an error if the ISA is not supported.
>
> Thanks
> Sri
>
>>
>> For all this you certainly need testcases.
>>
>> Jakub
