[clang] Don't modify OptRemark if the argument is not relevant

[aeubanks]

Summary

A followup to D110201.

For example, we'd set OptimizationRemarkMissed's Regex to '.*' when
encountering -Rpass. Normally this doesn't actually affect remarks we
emit because in clang::ProcessWarningOptions() we'll separately look at
all -R arguments and turn on/off corresponding diagnostic groups.
However, this is reproducible with -round-trip-args.

[dblaikie]

It'd be good to understand/document (maybe document in the form of a test if possible) how downstream users are relying on this - perhaps it's not a valid reliance and we shouldn't maintain compatibility? Maybe it is and we should ensure some test coverage of the sort of use case we're supporting, if possible.

[aeubanks]

In D110673#3029253, @dblaikie wrote:

It'd be good to understand/document (maybe document in the form of a test if possible) how downstream users are relying on this - perhaps it's not a valid reliance and we shouldn't maintain compatibility? Maybe it is and we should ensure some test coverage of the sort of use case we're supporting, if possible.

@JamesNagurne, given the description could you figure out why the test was failing for you downstream? I don't think I can show any difference upstream, but perhaps we should wait to see if @JamesNagurne can shed some more insight.

However, this logic is clearly wrong and just happens to work due to what I described in the description.

[JamesNagurne]

I'll take a quick look tomorrow, but the general idea is that on calling ParseOptimizationRemark on line 1909 with a -cc1 command line containing -Rpass=inline -Rno-pass, Opts.OptimizationRemarkMissed and Opts.OptimizationRemarkAnalysis are set to valid patterns (Regex is ".*", Pattern is "", and Kind is RK_Missing). This happens around line 1909 in the change set.

This configuration makes it into the LLVM backend. When prompted by specific calls to llvm::shouldInline (or something similar, can't remember the spelling off hand), emits an optimization-missed remark when one of the functions in the test's IR is not inlined.

"...in clang::ProcessWarningOptions() we'll separately look at all -R arguments and turn on/off corresponding diagnostic groups."

Why would -Rno-pass turn off "pass-missed" or "pass-analysis" diagnostic groups? That seems counterintuitive. They seem to be different groups, considering how they're used in the backend.

[aeubanks]

In D110673#3029438, @JamesNagurne wrote:

I'll take a quick look tomorrow, but the general idea is that on calling ParseOptimizationRemark on line 1909 with a -cc1 command line containing -Rpass=inline -Rno-pass, Opts.OptimizationRemarkMissed and Opts.OptimizationRemarkAnalysis are set to valid patterns (Regex is ".*", Pattern is "", and Kind is RK_Missing). This happens around line 1909 in the change set.

This configuration makes it into the LLVM backend. When prompted by specific calls to llvm::shouldInline (or something similar, can't remember the spelling off hand), emits an optimization-missed remark when one of the functions in the test's IR is not inlined.

"...in clang::ProcessWarningOptions() we'll separately look at all -R arguments and turn on/off corresponding diagnostic groups."

Why would -Rno-pass turn off "pass-missed" or "pass-analysis" diagnostic groups? That seems counterintuitive. They seem to be different groups, considering how they're used in the backend.

It's not that -Rno-pass turns off "pass-missed" or "pass-analysis", it's that -Rpass turns on the "pass" diagnostic groups, and the "pass-missed" or "pass-analysis" groups are never turned on in the first place.

[JamesNagurne]

The trigger for the remark I'm seeing is llvm::shouldInline in InlineAdvisor.cpp ((https://github.com/llvm/llvm-project/blob/2240deb9766cc080b351016b0d7f975d7249b113/llvm/lib/Transforms/IPO/Inliner.cpp#L425) which is called in a top-level AlwaysInlinerLegacyPass

Clang Release build with LLVM_ENABLE_ASSERTIONS=ON

%clang_cc1 %s -Rpass=inline -Rno-pass -emit-llvm -o -

• ORE.emit is called, which checks 'enabled()' first
• enabled checks if any of CodeGenOpts.OptimizationRemarkAnalysis, OptimizationRemarkMissed, or OptimizationRemark have a valid pattern (https://github.com/llvm/llvm-project/blob/2240deb9766cc080b351016b0d7f975d7249b113/clang/lib/CodeGen/CodeGenAction.cpp#L73)
  • Analysis and Missed have valid patterns, while OptimizationRemark does not
  • This coincides with ParseOptimizationRemark returning a result with a valid pattern for only "pass-analysis" and "pass-missed"
• The resulting diagnostic is handled by clang::BackendConsumer::OptimizationRemarkHandler (https://github.com/llvm/llvm-project/blob/2240deb9766cc080b351016b0d7f975d7249b113/clang/lib/CodeGen/CodeGenAction.cpp#L708)
  • The regex is '.*', so the pattern check passes, and then call EmitOptimizationMessage
  • Diags (the DiagnosticEngine modified by clang::ProcessWarningOptions) is used to report the diagnostic, given a DebugLoc and an ID
  • Diags->getDiagnosticLevel(DiagID, Loc) is Remark, so the diagnostic is emitted

---

Nothing here seems out of place. Regarding ProcessWarningOptions:
I've noticed that our tools being validated are doing a -cc1 round trip due to having LLVM_ENABLE_ASSERTIONS set on ON (https://github.com/llvm/llvm-project/blob/7dffb8b4da530d481977e31f439a92c5f6f2174a/clang/lib/Frontend/CompilerInvocation.cpp#L642)

• -cc1 options are parsed and generate a temporary CompilerInvocation
• The temporary CompilerInvocation is used to generate a second list of arguments
• The second list of arguments are parsed into the real CompilerInvocation
• A third list of arguments are generated from the real CompilerInvocation and compared against the second set to ensure that they are identical

If I run clang -cc1 with -no-round-trip-args, I no longer see the remarks. Adding some prints for each argument list in the round trip, I get (ignoring options unrelated to diagnostics):

• Initial (CommandLineArgs): [-Rpass=inline, -Rno-pass]
• GeneratedArgs1: [-Rno-pass, -Rpass-missed=.*, -Rpass-analysis=.*, -fdiagnostics-hotness-threshold=0]
• GeneratedArgs2: [-Rno-pass, -Rpass-missed=.*, -Rpass-analysis=.*, -fdiagnostics-hotness-threshold=0]

So, the round-trip check passes, but it looks like some internal consistency has been violated because parsing the initial CommandLineArgs into a CompilerInvocation does not result in erroneous remarks, but re-generating and parsing the arguments again leads to a change in behavior.

[JamesNagurne]

Perhaps you could reproduce my error -round-trip-args on the -cc1 command line?

[aeubanks]

forcing the legacy PM plus -round-trip-args repros the issue. added a new PM equivalent RUN line (that does fail without this patch)

[aeubanks]

Thanks for investigating!

[JamesNagurne]

You're welcome! Surprising that no downstream clang was running with the old PM and assertions...

I guess we better get on the new PM soon.

clang/lib/Frontend/CompilerInvocation.cpp

if (A->getOption().matches(OPT_R_Joined)) {
       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
+        continue;
 
       if (Result.Kind == CodeGenOptions::RK_Disabled ||
           Result.Kind == CodeGenOptions::RK_DisabledEverything) {
         Result.Pattern = "";
         Result.Regex = nullptr;
       } else {
         InitializeResultPattern(A, ".*");
       }