Re: [PATCH][i386]Fix PR 57756

[Sriraman Tallam <tmsriram at google dot com>]

On Mon, Sep 23, 2013 at 4:09 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Sat, Sep 21, 2013 at 4:09 AM, Sriraman Tallam <tmsriram@google.com> wrote:
>> Forgot to add the test case. Patch updated.
>>
>> Sri
>>
>> On Fri, Sep 20, 2013 at 7:06 PM, Sriraman Tallam <tmsriram@google.com> wrote:
>>> On Wed, Aug 14, 2013 at 3:38 AM, Richard Biener
>>> <richard.guenther@gmail.com> wrote:
>>>> On Wed, Aug 14, 2013 at 2:02 AM, Sriraman Tallam <tmsriram@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
>>>>
>>>>

Attachment: pr57756.txt
Description: Text document