Ping.
On Wed, Apr 17, 2013 at 7:13 PM, Sriraman Tallam <tmsri...@google.com> wrote:
> +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
