This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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?

> >
> > 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' ......."?

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.

Thanks,
Richard.

> Martin
>
> >
> > Richard.
> >
> >> Martin
> >>
> >>>
> >>> Martin
> >>>
> >>>>
> >>>> Ramana
> >>>>>>
> >>>>>> Martin
> >>>>>>
> >>>>>
> >>>
> >>
>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]