Re: [PATCH] Come up with TARGET_GET_VALID_OPTION_VALUES option hook (PR driver/83193).

[Martin Liška <mliska at suse dot cz>]

On 08/31/2018 11:29 AM, Martin Liška wrote:
> On 08/30/2018 12:16 PM, Richard Biener wrote:
>> On Wed, Aug 29, 2018 at 2:47 PM Martin Liška <mliska@suse.cz> wrote:
>>>
>>> On 08/29/2018 01:06 PM, Richard Biener wrote:
>>>> On Mon, Aug 27, 2018 at 12:00 PM Martin Liška <mliska@suse.cz> wrote:
>>>>>
>>>>> On 08/13/2018 03:00 PM, Martin Liška wrote:
>>>>>> On 08/13/2018 02:54 PM, Ramana Radhakrishnan wrote:
>>>>>>> On Mon, Aug 13, 2018 at 1:49 PM, Martin Liška <mliska@suse.cz> wrote:
>>>>>>>> PING^1
>>>>>>>>
>>>>>>>> On 07/24/2018 02:05 PM, Martin Liška wrote:
>>>>>>>>> Hi.
>>>>>>>>>
>>>>>>>>> I'm sending updated version of the patch. It comes up with a new target common hook
>>>>>>>>> that provide option completion list. It's used both in --help=target and with --completion
>>>>>>>>> option. I implemented support for -match and -mtune for i386 target.
>>>>>>>>>
>>>>>>>>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>>>>>>
>>>>>>>
>>>>>>> Err I don't maintain the x86 backend to review this effectively. In an
>>>>>>> ideal world you would have split this into 2 patches 1 for the common
>>>>>>> parts and 1 for x86 and CC'd the relevant x86 maintainers. I'm not
>>>>>>> clear what you are looking for here from me :-/
>>>>>>
>>>>>> Sorry, I was not clear. I would like to hear from ARM's folks that interface
>>>>>> design is fine for them? I know a global reviewer will have to approve that.
>>>>>
>>>>> I'm CC'ing Jakub and Richard who can help us with the new target hook infrastructure.
>>>>
>>>> +vec<const char *>
>>>> +ix86_get_valid_option_values (int option_code, const char *prefix)
>>>> +{
>>>>
>>>> prefix isn't used - why does that not fail bootstrap?
>>>
>>> Will add ATTRIBUTE_UNUSED.
>>>
>>>  It requires documentation
>>>> that honoring prefix isn't required and callers have to deal with
>>>
>>> It's more detail described in common-target.def:
>>>
>>> 'The hook is used for options that have a non-trivial list of\
>>>  possible option values.  OPTION_CODE is option code of opt_code\
>>>  enum type.  PREFIX is used for bash completion and allows an implementation\
>>>  to return more specific completion based on the prefix.  All string values\
>>>  should be allocated from heap memory and consumers should release them.'
>>>
>>> Should I copy it to the implementation.
>>>
>>>> that.  IMHO that
>>>> makes prefix useless?
>>>
>>> ARM folks requested that, they want to do a smart filtering for bash completion.
>>> It was there request.
>>
>> Ah, I see.  Based on your x86 example below I guess that generic code already
>> does prefix handling, yes?  I think that's something that should be documented,
>> that is, "The result will be pruned to cases with PREFIX if not NULL" or so?
> 
> Done.
> 
>>
>>>>
>>>> Unfortunately option_proposer::build_option_suggestions isn't documented
>>>> so I don't see whether it only receives target options.  If not then
>>>>
>>>> -           add_misspelling_candidates (m_option_suggestions, option,
>>>> -                                       opt_text);
>>>> +           {
>>>> +             vec<const char *> option_values
>>>> +               = targetm_common.get_valid_option_values (i, prefix);
>>>> +             if (!option_values.is_empty ())
>>>>
>>>> this should be guarded with a check for whether this is a target
>>>> option (CL_TARGET
>>>> in flags).
>>>
>>> Good point!
>>>
>>>  I wonder why misspellings are to be checked for the bash
>>>> completion case?
>>>
>>> Note that option_proposer::build_option_suggestions is shared infrastructure
>>> in between bash completion and misspellings.
>>>
>>> 2 examples:
>>>
>>> $ /xgcc -B. -march=znver2 -c /tmp/empty.c
>>> cc1: error: bad value (‘znver2’) for ‘-march=’ switch
>>> cc1: note: valid arguments to ‘-march=’ switch are:  ... did you mean ‘znver1’?
>>
>> Hmm, not very pretty ;)  If you do -march=bdver5, what will it print?
>> "... did you mean 'bdver1' ... did you mean 'bdver2' ......."?
> 
> Yep, it's not ideal, I created: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87165
> 
>>
>> Anyway, sort that out with David ;)
>>
>>> $  ./xgcc -B. --completion=-march=znver
>>> -march=znver1
>>>
>>>>
>>>> I also wonder why those target options could not simply be of Enum type and thus
>>>> be automatically handled?
>>>
>>> What a question, I asked the same one before I implemented that. It's because
>>> you have modifiers like: -march=armv8.3-a+simd+crypto+nofp for aarch64 target.
>>> For i386, it's also manually parsed because of tables that group
>>> options and various values:
>>>
>>> processor_target_table
>>> processor_alias_table
>>
>> Eh, OK.
>>
>>> I'm sending updated version of patch that I'm going to test.
>>
>> The middle-end changes (target hook addition) is OK.
>>
>> I guess the other non-target parts as well, we can improve over the
>> prettyness as followup.
>>
>> Please get target maintainer approval for the rest.
> 
> Good then, I'm sending final version of the patch that I've justed
> tested on x86_64-linux-gnu.

I was given offline approval from Honza about the i386-specific part.
I'm going to install that.

Martin

> 
> Martin
> 
>>
>> Thanks,
>> Richard.
>>
>>> Martin
>>>
>>>>
>>>> Richard.
>>>>
>>>>> Martin
>>>>>
>>>>>>
>>>>>> Martin
>>>>>>
>>>>>>>
>>>>>>> Ramana
>>>>>>>>>
>>>>>>>>> Martin
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>
>>>
>