Ping.
On Mon, Oct 7, 2013 at 1:27 PM, Sriraman Tallam <tmsri...@google.com> wrote:
> I have updated the patch with one more test.
>
> Thanks
> Sri
>
> On Thu, Oct 3, 2013 at 5:02 PM, Sriraman Tallam <tmsri...@google.com> wrote:
>> On Mon, Sep 23, 2013 at 4:09 AM, Richard Biener
>> <richard.guent...@gmail.com> wrote:
>>> On Sat, Sep 21, 2013 at 4:09 AM, Sriraman Tallam <tmsri...@google.com>
>>> wrote:
>>>> Forgot to add the test case. Patch updated.
>>>>
>>>> Sri
>>>>
>>>> On Fri, Sep 20, 2013 at 7:06 PM, Sriraman Tallam <tmsri...@google.com>
>>>> wrote:
>>>>> On Wed, Aug 14, 2013 at 3:38 AM, Richard Biener
>>>>> <richard.guent...@gmail.com> wrote:
>>>>>> On Wed, Aug 14, 2013 at 2:02 AM, Sriraman Tallam <tmsri...@google.com>
>>>>>> wrote:
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> I have attached a patch to fix PR57756. Description: The
>>>>>>> following program,
>>>>>>>
>>>>>>> __attribute__((always_inline,target("sse4.2")))
>>>>>>> __inline int callee ()
>>>>>>> {
>>>>>>> return 0;
>>>>>>> }
>>>>>>>
>>>>>>> __attribute__((target("sse")))
>>>>>>> __inline int caller ()
>>>>>>> {
>>>>>>> return callee();
>>>>>>> }
>>>>>>>
>>>>>>> does not generate an error and callee is inlined into caller. This is
>>>>>>> because callee has a higher target ISA. Interchanging the position of
>>>>>>> caller and callee will generate the correct error. Also, removing the
>>>>>>> target attribute from caller will generate the error.
>>>>>>>
>>>>>>> The reason for the bug is that when the caller's target options are
>>>>>>> processed, global_options contain the ISA flags of the callee
>>>>>>> (previously processed) and doing this in i386-common.c, where opts is
>>>>>>> global_options:
>>>>>>>
>>>>>>> opts->x_ix86_isa_flags |= OPTION_MASK_ISA_SSE_SET;
>>>>>>>
>>>>>>> is not changing global options. The fix is to reset global_options to
>>>>>>> the default each time a new target option needs to be processed.
>>>>>>
>>>>>>
>>>>>> Shouldn't ix86_valid_target_attribute_tree be refactored to work on a
>>>>>> selected
>>>>>> opt structure rather than global_options?
>>>>>
>>>>> I have attached a patch that does the refactoring of
>>>>> ix86_valid_target_attribute_tree. In that process I had to refactor
>>>>> ix86_valid_target_attribute_inner_p and ix86_option_override_internal.
>>>>> This made it a large patch because global_options structure is touched
>>>>> by these functions in multiple places and through vvarious aliases
>>>>> like ix86_arch_string for instance. In effect, this frees these
>>>>> functions from depending on global_options and the code in
>>>>> ix86_alid_target_attribute_p shows how the original bug can be fixed
>>>>> by just using a different gcc_options structure.
>>>
>>> This looks nice. I suppose you grepped for effected targets and rs6000
>>> was the only one besides i386.
>>>
>>> This is ok given target maintainers do not object within 24h and the test
>>> successfully bootstrapped and passed regression tests.
>>
>> I changed this patch quite a bit after I realized I missed many other
>> places where global_options field was accessed. The two specific
>> changes are:
>>
>> * Particularly, ix86_function_specific_save and
>> ix86_function_specific_restore had to be changed to copy to a specific
>> gcc_options structure than to global_options.
>> * Function ix86_option_override_internal accesses global_options via
>> macros, like TARGET_64BIT etc. This had to be changed. So, I created
>> new macros to accept a parameter and named them like TARGET_64BIT_P
>> and replaced all the accesses in i386.c
>>
>> I also marked as TODO a copy in function ix86_function_specific_save.
>> Here, the cl_target_option structure has a ix86_target_flags_explicit
>> field whereas the global_options structure does not have one.
>> Previously, this used to get saved to the global_options target_flags
>> field but this did not make sense to me. I have commented this line
>> out. Please let me know if a new field needs to be added.
>>
>> This patch passes bootstrap and all tests. Please take another look.
>>
>> Sri
>>
>>>
>>> Thanks,
>>> Richard.
>>>
>>>>> Please review.
>>>>>
>>>>> Thanks
>>>>> Sri
>>>>>
>>>>>
>>>>>
>>>>>> Richard.
>>>>>>
>>>>>>>
>>>>>>> Patch ok?
>>>>>>>
>>>>>>> Thanks
>>>>>>> Sri
>>>>>>
>>>>>>
