[clang][cli] Store additional optimization remarks info

[jansvoboda11]

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.

[jansvoboda11]

Rename variable

[dexonsmith]

clang/include/clang/Basic/CodeGenOptions.h

318–319

Can this just be a Kind field in RemarkPattern?

[jansvoboda11]

Put RemarkKind in RemarkPattern.

[jansvoboda11]

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(...); }?

[dexonsmith]

In D96847#2574408, @jansvoboda11 wrote:

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(...)?

[jansvoboda11]

Rename RemarkPattern to OptRemark, simplify the API usage.

[jansvoboda11]

Comment wording.

[jansvoboda11]

In D96847#2576369, @dexonsmith wrote:

In D96847#2574408, @jansvoboda11 wrote:

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.

[jansvoboda11]

Ping.

[dexonsmith]

LGTM.

clang/include/clang/Basic/CodeGenOptions.h

public:
 
   /// The format used for serializing remarks (default: YAML)
   std::string OptRecordFormat;
 
   /// The name of the partition that symbols are assigned to, specified with
   /// -fsymbol-partition (see https://lld.llvm.org/Partitions.html).
   std::string SymbolPartition;
 
-  /// Regular expression and the string it was created from.
-  struct RemarkPattern {
+  enum RemarkKind {
+    RK_Missing,            // Remark argument not present on the command line.
+    RK_Enabled,            // Remark enabled via '-Rgroup'.
+    RK_EnabledEverything,  // Remark enabled via '-Reverything'.
+    RK_Disabled,           // Remark disabled via '-Rno-group'.
+    RK_DisabledEverything, // Remark disabled via '-Rno-everything'.
+    RK_WithPattern,        // Remark pattern specified via '-Rgroup=regexp'.
+  };
+
+  /// Optimization remark with an optional regular expression pattern.
+  struct OptRemark {
+    RemarkKind Kind;
     std::string Pattern;
     std::shared_ptr<llvm::Regex> Regex;
 
-    explicit operator bool() const { return Regex != nullptr; }
+    /// By default, optimization remark is missing.
+    OptRemark() : Kind(RK_Missing), Pattern(""), Regex(nullptr) {}
 
-    llvm::Regex *operator->() const { return Regex.get(); }
+    /// Returns true iff the optimization remark holds a valid regular
+    /// expression.
+    bool hasValidPattern() const { return Regex != nullptr; }
+
+    /// Matches the given string against the regex, if there is some.
+    bool patternMatches(StringRef String) const {
+      return hasValidPattern() && Regex->match(String);
+    }
   };
 
-  /// Regular expression to select optimizations for which we should enable
-  /// optimization remarks. Transformation passes whose name matches this
-  /// expression (and support this feature), will emit a diagnostic
-  /// whenever they perform a transformation. This is enabled by the
-  /// -Rpass=regexp flag.
-  RemarkPattern OptimizationRemarkPattern;
-
-  /// Regular expression to select optimizations for which we should enable
-  /// missed optimization remarks. Transformation passes whose name matches this
-  /// expression (and support this feature), will emit a diagnostic
-  /// whenever they tried but failed to perform a transformation. This is
-  /// enabled by the -Rpass-missed=regexp flag.
-  RemarkPattern OptimizationRemarkMissedPattern;
-
-  /// Regular expression to select optimizations for which we should enable
-  /// optimization analyses. Transformation passes whose name matches this
-  /// expression (and support this feature), will emit a diagnostic
-  /// whenever they want to explain why they decided to apply or not apply
-  /// a given transformation. This is enabled by the -Rpass-analysis=regexp
-  /// flag.
-  RemarkPattern OptimizationRemarkAnalysisPattern;
+  /// Selected optimizations for which we should enable optimization remarks.
+  /// Transformation passes whose name matches the contained (optional) regular
+  /// expression (and support this feature), will emit a diagnostic whenever
+  /// they perform a transformation.
+  OptRemark OptimizationRemark;
+
+  /// Selected optimizations for which we should enable missed optimization
+  /// remarks. Transformation passes whose name matches the contained (optional)
+  /// regular expression (and support this feature), will emit a diagnostic
+  /// whenever they tried but failed to perform a transformation.
+  OptRemark OptimizationRemarkMissed;
+
+  /// Selected optimizations for which we should enable optimization analyses.
+  /// Transformation passes whose name matches the contained (optional) regular
+  /// expression (and support this feature), will emit a diagnostic whenever
+  /// they want to explain why they decided to apply or not apply a given
+  /// transformation.
+  OptRemark OptimizationRemarkAnalysis;
 
   /// Set of files defining the rules for the symbol rewriting.
   std::vector<std::string> RewriteMapFiles;
 
   /// Set of sanitizer checks that are non-fatal (i.e. execution should be
   /// continued when possible).
   SanitizerSet SanitizeRecover;
 

clang/lib/CodeGen/CodeGenAction.cpp

namespace clang {
   class ClangDiagnosticHandler final : public DiagnosticHandler {
   public:
     ClangDiagnosticHandler(const CodeGenOptions &CGOpts, BackendConsumer *BCon)
         : CodeGenOpts(CGOpts), BackendCon(BCon) {}
 
     bool handleDiagnostics(const DiagnosticInfo &DI) override;
 
     bool isAnalysisRemarkEnabled(StringRef PassName) const override {
-      return (CodeGenOpts.OptimizationRemarkAnalysisPattern &&
-              CodeGenOpts.OptimizationRemarkAnalysisPattern->match(PassName));
+      return CodeGenOpts.OptimizationRemarkAnalysis.patternMatches(PassName);
     }
     bool isMissedOptRemarkEnabled(StringRef PassName) const override {
-      return (CodeGenOpts.OptimizationRemarkMissedPattern &&
-              CodeGenOpts.OptimizationRemarkMissedPattern->match(PassName));
+      return CodeGenOpts.OptimizationRemarkMissed.patternMatches(PassName);
     }
     bool isPassedOptRemarkEnabled(StringRef PassName) const override {
-      return (CodeGenOpts.OptimizationRemarkPattern &&
-              CodeGenOpts.OptimizationRemarkPattern->match(PassName));
+      return CodeGenOpts.OptimizationRemark.patternMatches(PassName);
     }
 
     bool isAnyRemarkEnabled() const override {
-      return (CodeGenOpts.OptimizationRemarkAnalysisPattern ||
-              CodeGenOpts.OptimizationRemarkMissedPattern ||
-              CodeGenOpts.OptimizationRemarkPattern);
+      return CodeGenOpts.OptimizationRemarkAnalysis.hasValidPattern() ||
+             CodeGenOpts.OptimizationRemarkMissed.hasValidPattern() ||
+             CodeGenOpts.OptimizationRemark.hasValidPattern();
     }
 
   private:
     const CodeGenOptions &CodeGenOpts;
     BackendConsumer *BackendCon;
   };
 
   static void reportOptRecordError(Error E, DiagnosticsEngine &Diags,
void BackendConsumer::OptimizationRemarkHandler(
     const llvm::DiagnosticInfoOptimizationBase &D) {
   // Without hotness information, don't show noisy remarks.
   if (D.isVerbose() && !D.getHotness())
     return;
 
   if (D.isPassed()) {
     // Optimization remarks are active only if the -Rpass flag has a regular
     // expression that matches the name of the pass name in \p D.
-    if (CodeGenOpts.OptimizationRemarkPattern &&
-        CodeGenOpts.OptimizationRemarkPattern->match(D.getPassName()))
+    if (CodeGenOpts.OptimizationRemark.patternMatches(D.getPassName()))
       EmitOptimizationMessage(D, diag::remark_fe_backend_optimization_remark);
   } else if (D.isMissed()) {
     // Missed optimization remarks are active only if the -Rpass-missed
     // flag has a regular expression that matches the name of the pass
     // name in \p D.
-    if (CodeGenOpts.OptimizationRemarkMissedPattern &&
-        CodeGenOpts.OptimizationRemarkMissedPattern->match(D.getPassName()))
+    if (CodeGenOpts.OptimizationRemarkMissed.patternMatches(D.getPassName()))
       EmitOptimizationMessage(
           D, diag::remark_fe_backend_optimization_remark_missed);
   } else {
     assert(D.isAnalysis() && "Unknown remark type");
 
     bool ShouldAlwaysPrint = false;
     if (auto *ORA = dyn_cast<llvm::OptimizationRemarkAnalysis>(&D))
       ShouldAlwaysPrint = ORA->shouldAlwaysPrint();
 
     if (ShouldAlwaysPrint ||
-        (CodeGenOpts.OptimizationRemarkAnalysisPattern &&
-         CodeGenOpts.OptimizationRemarkAnalysisPattern->match(D.getPassName())))
+        CodeGenOpts.OptimizationRemarkAnalysis.patternMatches(D.getPassName()))
       EmitOptimizationMessage(
           D, diag::remark_fe_backend_optimization_remark_analysis);
   }
 }
 
 void BackendConsumer::OptimizationRemarkHandler(
     const llvm::OptimizationRemarkAnalysisFPCommute &D) {
   // Optimization analysis remarks are active if the pass name is set to
   // llvm::DiagnosticInfo::AlwasyPrint or if the -Rpass-analysis flag has a
   // regular expression that matches the name of the pass name in \p D.
 
   if (D.shouldAlwaysPrint() ||
-      (CodeGenOpts.OptimizationRemarkAnalysisPattern &&
-       CodeGenOpts.OptimizationRemarkAnalysisPattern->match(D.getPassName())))
+      CodeGenOpts.OptimizationRemarkAnalysis.patternMatches(D.getPassName()))
     EmitOptimizationMessage(
         D, diag::remark_fe_backend_optimization_remark_analysis_fpcommute);
 }
 
 void BackendConsumer::OptimizationRemarkHandler(
     const llvm::OptimizationRemarkAnalysisAliasing &D) {
   // Optimization analysis remarks are active if the pass name is set to
   // llvm::DiagnosticInfo::AlwasyPrint or if the -Rpass-analysis flag has a
   // regular expression that matches the name of the pass name in \p D.
 
   if (D.shouldAlwaysPrint() ||
-      (CodeGenOpts.OptimizationRemarkAnalysisPattern &&
-       CodeGenOpts.OptimizationRemarkAnalysisPattern->match(D.getPassName())))
+      CodeGenOpts.OptimizationRemarkAnalysis.patternMatches(D.getPassName()))
     EmitOptimizationMessage(
         D, diag::remark_fe_backend_optimization_remark_analysis_aliasing);
 }
 
 void BackendConsumer::OptimizationFailureHandler(
     const llvm::DiagnosticInfoOptimizationFailure &D) {
   EmitOptimizationMessage(D, diag::warn_fe_backend_optimization_failure);
 }

clang/lib/Frontend/CompilerInvocation.cpp

Diags->Report(diag::err_analyzer_config_invalid_input) << "ctu-dir"
                                                            << "a filename";
 
   if (!AnOpts.ModelPath.empty() &&
       !llvm::sys::fs::is_directory(AnOpts.ModelPath))
     Diags->Report(diag::err_analyzer_config_invalid_input) << "model-path"
                                                            << "a filename";
 }
 
-/// Create a new Regex instance out of the string value in \p RpassArg.
-/// It returns the string and a pointer to the newly generated Regex instance.
-static CodeGenOptions::RemarkPattern
-GenerateOptimizationRemarkRegex(DiagnosticsEngine &Diags, ArgList &Args,
-                                Arg *RpassArg) {
-  StringRef Val = RpassArg->getValue();
+/// Generate a remark argument. This is an inverse of `ParseOptimizationRemark`.
+static void
+GenerateOptimizationRemark(SmallVectorImpl<const char *> &Args,
+                           CompilerInvocation::StringAllocator SA,
+                           OptSpecifier OptEQ, StringRef Name,
+                           const CodeGenOptions::OptRemark &Remark) {
+  if (Remark.hasValidPattern()) {
+    GenerateArg(Args, OptEQ, Remark.Pattern, SA);
+  } else if (Remark.Kind == CodeGenOptions::RK_Enabled) {
+    GenerateArg(Args, OPT_R_Joined, Name, SA);
+  } else if (Remark.Kind == CodeGenOptions::RK_Disabled) {
+    GenerateArg(Args, OPT_R_Joined, StringRef("no-") + Name, SA);
+  }
+};
+
+/// Parse a remark command line argument. It may be missing, disabled/enabled by
+/// '-R[no-]group' or specified with a regular expression by '-Rgroup=regexp'.
+/// On top of that, it can be disabled/enabled globally by '-R[no-]everything'.
+static CodeGenOptions::OptRemark
+ParseOptimizationRemark(DiagnosticsEngine &Diags, ArgList &Args,
+                        OptSpecifier OptEQ, StringRef Name) {
+  CodeGenOptions::OptRemark Result;
+
+  auto InitializeResultPattern = [&Diags, &Args, &Result](const Arg *A) {
+    Result.Pattern = A->getValue();
+
     std::string RegexError;
-  std::shared_ptr<llvm::Regex> Pattern = std::make_shared<llvm::Regex>(Val);
-  if (!Pattern->isValid(RegexError)) {
+    Result.Regex = std::make_shared<llvm::Regex>(Result.Pattern);
+    if (!Result.Regex->isValid(RegexError)) {
       Diags.Report(diag::err_drv_optimization_remark_pattern)
-        << RegexError << RpassArg->getAsString(Args);
-    Pattern.reset();
+          << RegexError << A->getAsString(Args);
+      return false;
     }
-  return {std::string(Val), Pattern};
+
+    return true;
+  };
+
+  for (Arg *A : Args) {
+    if (A->getOption().matches(OPT_R_Joined)) {
+      StringRef Value = A->getValue();
+
+      if (Value == Name)
+        Result.Kind = CodeGenOptions::RK_Enabled;
+      else if (Value == "everything")
+        Result.Kind = CodeGenOptions::RK_EnabledEverything;
+      else if (Value.split('-') == std::make_pair(StringRef("no"), Name))
+        Result.Kind = CodeGenOptions::RK_Disabled;
+      else if (Value == "no-everything")
+        Result.Kind = CodeGenOptions::RK_DisabledEverything;
+    } else if (A->getOption().matches(OptEQ)) {
+      Result.Kind = CodeGenOptions::RK_WithPattern;
+      if (!InitializeResultPattern(A))
+        return CodeGenOptions::OptRemark();
+    }
+  }
+
+  if (Result.Kind == CodeGenOptions::RK_Disabled ||
+      Result.Kind == CodeGenOptions::RK_DisabledEverything) {
+    Result.Pattern = "";
+    Result.Regex = nullptr;
+  }
+
+  return Result;
 }
 
 static bool parseDiagnosticLevelMask(StringRef FlagName,
                                      const std::vector<std::string> &Levels,
                                      DiagnosticsEngine &Diags,
                                      DiagnosticLevelMask &M) {
   bool Success = true;
   for (const auto &Level : Levels) {
if (Opts.EnableAIXExtendedAltivecABI)
     GenerateArg(Args, OPT_mabi_EQ_vec_extabi, SA);
 
   if (!Opts.OptRecordPasses.empty())
     GenerateArg(Args, OPT_opt_record_passes, Opts.OptRecordPasses, SA);
 
   if (!Opts.OptRecordFormat.empty())
     GenerateArg(Args, OPT_opt_record_format, Opts.OptRecordFormat, SA);
 
-  if (Opts.OptimizationRemarkPattern)
-    GenerateArg(Args, OPT_Rpass_EQ, Opts.OptimizationRemarkPattern.Pattern, SA);
+  GenerateOptimizationRemark(Args, SA, OPT_Rpass_EQ, "pass",
+                             Opts.OptimizationRemark);
+
+  GenerateOptimizationRemark(Args, SA, OPT_Rpass_missed_EQ, "pass-missed",
+                             Opts.OptimizationRemarkMissed);
 
-  if (Opts.OptimizationRemarkMissedPattern)
-    GenerateArg(Args, OPT_Rpass_missed_EQ,
-                Opts.OptimizationRemarkMissedPattern.Pattern, SA);
-
-  if (Opts.OptimizationRemarkAnalysisPattern)
-    GenerateArg(Args, OPT_Rpass_analysis_EQ,
-                Opts.OptimizationRemarkAnalysisPattern.Pattern, SA);
+  GenerateOptimizationRemark(Args, SA, OPT_Rpass_analysis_EQ, "pass-analysis",
+                             Opts.OptimizationRemarkAnalysis);
 
   GenerateArg(Args, OPT_fdiagnostics_hotness_threshold_EQ,
               Opts.DiagnosticsHotnessThreshold
                   ? Twine(*Opts.DiagnosticsHotnessThreshold)
                   : "auto",
               SA);
 
   for (StringRef Sanitizer : serializeSanitizerKinds(Opts.SanitizeRecover))
if (Arg *A = Args.getLastArg(OPT_opt_record_passes)) {
     NeedLocTracking = true;
   }
 
   if (Arg *A = Args.getLastArg(OPT_opt_record_format)) {
     Opts.OptRecordFormat = A->getValue();
     NeedLocTracking = true;
   }
 
-  if (Arg *A = Args.getLastArg(OPT_Rpass_EQ)) {
-    Opts.OptimizationRemarkPattern =
-        GenerateOptimizationRemarkRegex(Diags, Args, A);
-    NeedLocTracking = true;
-  }
+  Opts.OptimizationRemark =
+      ParseOptimizationRemark(Diags, Args, OPT_Rpass_EQ, "pass");
 
-  if (Arg *A = Args.getLastArg(OPT_Rpass_missed_EQ)) {
-    Opts.OptimizationRemarkMissedPattern =
-        GenerateOptimizationRemarkRegex(Diags, Args, A);
-    NeedLocTracking = true;
-  }
+  Opts.OptimizationRemarkMissed =
+      ParseOptimizationRemark(Diags, Args, OPT_Rpass_missed_EQ, "pass-missed");
 
-  if (Arg *A = Args.getLastArg(OPT_Rpass_analysis_EQ)) {
-    Opts.OptimizationRemarkAnalysisPattern =
-        GenerateOptimizationRemarkRegex(Diags, Args, A);
-    NeedLocTracking = true;
-  }
+  Opts.OptimizationRemarkAnalysis = ParseOptimizationRemark(
+      Diags, Args, OPT_Rpass_analysis_EQ, "pass-analysis");
+
+  NeedLocTracking |= Opts.OptimizationRemark.hasValidPattern() ||
+                     Opts.OptimizationRemarkMissed.hasValidPattern() ||
+                     Opts.OptimizationRemarkAnalysis.hasValidPattern();
 
   bool UsingSampleProfile = !Opts.SampleProfileFile.empty();
   bool UsingProfile = UsingSampleProfile ||
       (Opts.getProfileUse() != CodeGenOptions::ProfileNone);
 
   if (Opts.DiagnosticsWithHotness && !UsingProfile &&
       // An IR file will contain PGO as metadata
       IK.getLanguage() != Language::LLVM_IR)
#undef DIAG_OPTION_WITH_MARSHALLING
 
   for (const auto &Warning : Opts.Warnings) {
     // This option is automatically generated from UndefPrefixes.
     if (Warning == "undef-prefix")
       continue;
     Args.push_back(SA(StringRef("-W") + Warning));
   }
 
-  for (const auto &Remark : Opts.Remarks)
+  for (const auto &Remark : Opts.Remarks) {
+    // These arguments are generated from OptimizationRemark fields of
+    // CodeGenOptions.
+    StringRef IgnoredRemarks[] = {"pass",          "no-pass",
+                                  "pass-analysis", "no-pass-analysis",
+                                  "pass-missed",   "no-pass-missed"};
+    if (llvm::is_contained(IgnoredRemarks, Remark))
+      continue;
+
     Args.push_back(SA(StringRef("-R") + Remark));
   }
+}
 
 bool clang::ParseDiagnosticArgs(DiagnosticOptions &Opts, ArgList &Args,
                                 DiagnosticsEngine *Diags,
                                 bool DefaultDiagColor) {
   Optional<DiagnosticsEngine> IgnoringDiags;
   if (!Diags) {
     IgnoringDiags.emplace(new DiagnosticIDs(), new DiagnosticOptions(),
                           new IgnoringDiagConsumer());