Page MenuHomePhabricator

Add support for options with two flags for controlling the same field.
ClosedPublic

Authored by jansvoboda11 on Jul 2 2020, 11:58 AM.

Details

Summary

This enables automatically parsing and generating CC1 arguments for options where two flags control the same field, e.g. -fexperimental-new-pass-manager and -fno-experimental new pass manager.

Diff Detail

Event Timeline

dang created this revision.Jul 2 2020, 11:58 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 2 2020, 11:58 AM
dexonsmith requested changes to this revision.Jul 2 2020, 7:14 PM
dexonsmith added inline comments.
clang/lib/Frontend/CompilerInvocation.cpp
146

I'm not sure if removing the llvm:: namespace is intentional here, but if so please do it in a separate NFC patch to avoid adding noise in this one.

181–183

This should be:

if (Arg *A = Args.getLastArg(PosOpt, NegOpt))
  return A->getOption().matches(PosOpt);
return None;

Note that hasArg and hasFlag both resolve to getLastArg, so calling them one after another would be unfortunate.

... but I'm not even sure where NegOpt is coming from here, that looks like it hasn't been passed in. I think you need to change the signature to something like this:

static llvm::Optional<bool>
normalizeSimpleFlag(OptSpecifier Opt,
                    Optional<OptSpecifier> NegOpt,
                    unsigned TableIndex,
                    const ArgList &Args,
                    DiagnosticsEngine &Diags) {
  // Handle case without a `no-*` flag.
  if (!NegOpt)
    return Args.hasArg(Opt);

  // Handle case with a `no-*` flag.
  return Args.hasFlagAsOptional(Opt, *NegOpt);
}

It's possible you'll need to split up OPTION_WITH_MARSHALLING into two disjoint lists of options:

  • The list of options that can't be negated.
  • The list of options that can be negated, calling a different macro that adds macro arguments for the CANCEL_ID and CANCEL_SPELLING. For the denormalizer you might also need CANCEL_VALUE.
  • Note: the negating options themselves wouldn't be visited in either list.
  • Note: the (de)normalizer APIs would ideally work naturally for something like -farg=val1 vs. -farg=val2 vs. -fno-arg.
4048–4061

I realize this commit doesn't introduce it, but it seems unfortunate to call EXTRACTOR twice. Maybe in a follow-up or prep commit you can fix that... maybe something like this?

if ((FLAGS)&options::CC1Option) {
  const auto &Extracted = EXTRACTOR(this->KEYPATH);
  if (ALWAYS_EMIT || Extracted != DEFAULT_VALUE)
    DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, EXTRACTOR(this->KEYPATH));
}
llvm/include/llvm/Option/OptParser.td
181

Nit: missing a space in ... Normalizer ="norma...; same typo repeats below.

This revision now requires changes to proceed.Jul 2 2020, 7:14 PM
dang updated this revision to Diff 275586.Jul 5 2020, 10:21 PM

Revert accidental namespace removal.

dang marked 5 inline comments as done.Jul 5 2020, 11:05 PM
dang added inline comments.
clang/lib/Frontend/CompilerInvocation.cpp
146

Yes it was accidental sorry about that.

181–183

NegOpt is passed in via the template parameter, the only weird bit about it is that the option name (for example OPT_fno_experimental_pass_manager) is constructed in tablegen by hardcoding the OPT_ prefix. What is currently there supports the case where the positive option takes a value and the negative one doesn't by using different normalizer/denormalizer pairs for the positive and the negative option. The bad thing about the current setup is that both options have identical normalizers, but I felt that was less bad than splitting the list of options and using different macros.

4048–4061

Yes I can do that of course. Although EXTRACTOR is meant to be very cheap and in most cases it expands to just this->KEYPATH

dang updated this revision to Diff 275591.Jul 5 2020, 11:06 PM
dang marked an inline comment as done.

Address some code review feedback.

dang marked an inline comment as done.Jul 6 2020, 10:12 AM
dang added inline comments.
clang/lib/Frontend/CompilerInvocation.cpp
4048–4061

See D83211

dang marked an inline comment as not done.Jul 6 2020, 10:14 AM
dang updated this revision to Diff 276007.Jul 7 2020, 5:10 AM

Make mergers use values directly instead of constant references

dang updated this revision to Diff 276009.Jul 7 2020, 5:13 AM

Rebase on top of some changes to parent patches.

dang updated this revision to Diff 276400.Jul 8 2020, 6:09 AM

Split into two macro kinds.

dang marked 3 inline comments as done.Jul 9 2020, 2:46 AM
dang updated this revision to Diff 280945.Jul 27 2020, 9:40 AM

Update with latest changes

dexonsmith accepted this revision.Aug 28 2020, 10:51 AM
This revision is now accepted and ready to land.Aug 28 2020, 10:51 AM
jansvoboda11 commandeered this revision.Nov 11 2020, 5:10 AM
jansvoboda11 added a reviewer: dang.
jansvoboda11 added a subscriber: jansvoboda11.

Taking control of this revision, as Daniel is no longer involved.

Rebase on top of recent changes.

Apply unique_ptr workaround for older Clang versions

This revision was landed with ongoing or failed builds.Nov 16 2020, 1:24 AM
This revision was automatically updated to reflect the committed changes.