This is an archive of the discontinued LLVM Phabricator instance.

[clang][cli] Store additional optimization remarks info
ClosedPublic

Authored by jansvoboda11 on Feb 17 2021, 1:08 AM.

Details

Summary

After a revision of D96274 changed DiagnosticOptions to not store all remark arguments as-written, it is no longer possible to reconstruct the arguments accurately from the class.

This is caused by the fact that for -Rpass=regexp and friends, DiagnosticOptions store only the group name pass and not regexp. This is the same representation used for the plain -Rpass argument.

Note that each argument must be generated exactly once in CompilerInvocation::generateCC1CommandLine, otherwise each subsequent call would produce more arguments than the previous one. Currently this works out because of the way RoundTrip splits the responsibilities for certain arguments based on what arguments were queried during parsing. However, this invariant breaks when we move to single round-trip for the whole CompilerInvocation.

This patch ensures that for one -Rpass=regexp argument, we don't generate two arguments (-Rpass from DiagnosticOptions and -Rpass=regexp from CodeGenOptions) by shifting the responsibility for handling both cases to CodeGenOptions. To distinguish between the cases correctly, additional information is stored in CodeGenOptions.

The CodeGenOptions parser of -Rpass[=regexp] arguments also looks at -Rno-pass and -R[no-]everything, which is necessary for generating the correct argument regardless of the ordering of CodeGenOptions/DiagnosticOptions parsing/generation.

Diff Detail

Event Timeline

jansvoboda11 requested review of this revision.Feb 17 2021, 1:08 AM
jansvoboda11 created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 17 2021, 1:08 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
jansvoboda11 edited the summary of this revision. (Show Details)Feb 17 2021, 1:09 AM

Rename variable

jansvoboda11 edited the summary of this revision. (Show Details)Feb 17 2021, 1:20 AM
dexonsmith added inline comments.Feb 17 2021, 1:53 PM
clang/include/clang/Basic/CodeGenOptions.h
310–311

Can this just be a Kind field in RemarkPattern?

Put RemarkKind in RemarkPattern.

That's a bit nicer.

Not sure if RemarkPattern is a good name now, as it may represent an optimization remark that doesn't have any pattern associated with it.
How about calling it OptimizationRemark and merging operator bool and operator -> into bool patternMatch(...) { return Pattern && Pattern.match(...); }?

That's a bit nicer.

Not sure if RemarkPattern is a good name now, as it may represent an optimization remark that doesn't have any pattern associated with it.
How about calling it OptimizationRemark and merging operator bool and operator -> into bool patternMatch(...) { return Pattern && Pattern.match(...); }?

Seems reasonable. Or matchesPattern(...)?

Rename RemarkPattern to OptRemark, simplify the API usage.

Comment wording.

That's a bit nicer.

Not sure if RemarkPattern is a good name now, as it may represent an optimization remark that doesn't have any pattern associated with it.
How about calling it OptimizationRemark and merging operator bool and operator -> into bool patternMatch(...) { return Pattern && Pattern.match(...); }?

Seems reasonable. Or matchesPattern(...)?

I chose patternMatches, which (IMO) suggests the pattern is contained within the object and the string to be matched is passed as an argument. To me, matchesPattern sounds like it's the other way around, which would contradict the code.

This revision is now accepted and ready to land.Feb 24 2021, 11:58 AM
This revision was landed with ongoing or failed builds.Feb 25 2021, 2:03 AM
This revision was automatically updated to reflect the committed changes.