This is an archive of the discontinued LLVM Phabricator instance.

[clang][cli] Do not marshall only CC1Option flags in BoolOption
ClosedPublic

Authored by jansvoboda11 on Dec 10 2020, 12:29 AM.

Details

Summary

We cannot be sure whether a flag is CC1Option inside the definition of BoolOption. Take the example below:

let Flags = [CC1Option] in {
  defm xxx : BoolOption<...>;
}

where TableGen applies Flags = [CC1Option] to the xxx and no_xxx records after they have been is fully declared by BoolOption.

For the refactored -f[no-]debug-pass-manager flags (see the diff), this means BoolOption never adds any marshalling info, as it doesn't see either of the flags as CC1Option.

For that reason, we should defensively append the marshalling information to both flags inside BoolOption. Now the check for CC1Option needs to happen later, in the parsing macro, when all TableGen logic has been resolved.

However, for some flags defined through BoolOption, we can run into issues:

// Options.td
def fenable_xxx : /* ... */;

// Both flags are CC1Option, the first is implied.
defm xxx : BoolOption<"xxx,
  "Opts.Xxx", DefaultsToFalse,
  ChangedBy<PosFlag, [CC1Option], "", [fenable_xxx]>,
  ResetBy<NegFlag, [CC1Option]>>;

When parsing clang -cc1 -fenable-xxx:

  • we run parsing for PosFlag:
    • set Opts.Xxx to default false,
    • discover PosFlag is implied by -fenable-xxx: set Opts.Xxx to true,
    • don't see -fxxx on command line: do nothing,
  • we run parsing for NegFlag:
    • set Opts.Xxx to default false,
    • discover NegFlag cannot be implied: do nothing,
    • don't see -fno-xxx on command line: do nothing.

Now we ended up with Opts.Xxx set to false instead of true. For this reason, we need to ensure to append the same ImpliedByAnyOf instance to both flags.

This means both parsing runs now behave identically (they set the same default value, run the same "implied by" check, and call makeBooleanOptionNormalizer that already has information on both flags, so it returns the same value in both calls).

The solution works well, but what might be confusing is this: you have defined a flag A that is not CC1Option, but can be implied by another flag B that is CC1Option:

  • if A is defined manually, it will never get implied, as the code never runs
def no_signed_zeros : Flag<["-"], "fno-signed-zeros">, Group<f_Group>, Flags<[]>,
  MarshallingInfoFlag<"LangOpts->NoSignedZero">, ImpliedByAnyOf<[menable_unsafe_fp_math]>;
  • if A is defined by BoolOption, it could get implied, as the code is run by its CC1Option counterpart:
defm signed_zeros : BoolOption<"signed-zeros",
  "LangOpts->NoSignedZero", DefaultsToFalse,
  ChangedBy<NegFlag, [], "Allow optimizations that ignore the sign of floating point zeros",
            [cl_no_signed_zeros, menable_unsafe_fp_math]>,
  ResetBy<PosFlag, [CC1Option]>, "f">, Group<f_Group>;

This is a surprising inconsistency.

One solution might be to somehow propagate the final Flags of the implied flag in ImpliedByAnyOf and check whether it has CC1Option in the parsing macro. However, I think it doesn't make sense to spend time thinking about a corner case that won't come up in real code.

An observation: it is unfortunate that the marshalling information is a part of the flag definition. If we represented it in a separate structure, we could avoid the "double parsing" problem by having a single source of truth. This would require a lot of additional work though.

Note: the original patch missed the CC1Option check in the parsing macro, making my reasoning here incomplete. Moreover, it contained a change to denormalization that wasn't necessarily related to these changes, so I've extracted that to a follow-up patch: D93094.

Diff Detail

Event Timeline

jansvoboda11 created this revision.Dec 10 2020, 12:29 AM
jansvoboda11 requested review of this revision.Dec 10 2020, 12:29 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 10 2020, 12:29 AM
jansvoboda11 edited the summary of this revision. (Show Details)Dec 10 2020, 1:06 AM

Fix comment typo

jansvoboda11 edited the summary of this revision. (Show Details)Dec 10 2020, 8:25 AM
jansvoboda11 edited the summary of this revision. (Show Details)
jansvoboda11 edited the summary of this revision. (Show Details)
jansvoboda11 planned changes to this revision.Dec 10 2020, 8:47 AM

Add forgotten CC1Option check in parsing macro, extract denormalizer changes

This revision is now accepted and ready to land.Dec 11 2020, 12:42 AM
jansvoboda11 requested review of this revision.Dec 11 2020, 1:16 AM
jansvoboda11 edited the summary of this revision. (Show Details)

@Bigcheese, can you please go through this again and let me know it it looks good to you now?

jansvoboda11 edited the summary of this revision. (Show Details)Dec 11 2020, 1:45 AM
This revision is now accepted and ready to land.Dec 15 2020, 8:41 AM
This revision was landed with ongoing or failed builds.Dec 16 2020, 12:51 AM
This revision was automatically updated to reflect the committed changes.