[Flang][Driver] Add support for Rpass and related options

[victorkingi]

Summary

Allow flang to report what optimizations were and were not done by the LLVM compiler optimizer.

A BackendRemarkConsumer was implemented in reference to
clang BackendConsumer.

Added required fields in flang/include/flang/Frontend/CodeGenOptions.h which include
the OptRemark fields, OptRemark struct and the RemarkKind enum class.

The parseOptimizationRemark and printRemarkOption functions created
are identical to the ones used in Clang.

Since Flang doesn't have a SourceManager instance in the BackendRemarkConsumer class,
we pass the absolute path and file name in flang/lib/Frontend/FrontendActions.cpp
emitOptimizationMessage function, then process it in flang/lib/Frontend/TextDiagnosticPrinter.cpp

Depends on D157410. That patch adds the R family of options to FlangOption and FC1Option in
clang/include/clang/Driver/Options.td

[victorkingi]

rpass flag now prints remarks when requested but does not display
the passName used, i.e [-Rpass=inline]

[victorkingi]

added test file optimization-remark.f90

[victorkingi]

added frontend forwarding test for Rpass flags

[victorkingi]

removed unused header includes

[kiranchandramohan]

rpass flag now prints remarks when requested but does not display
the passName used, i.e [-Rpass=inline]

I think the location information is also not printed. Please check the difference in implementation of the TextDiagnosticPrinter::HandleDiagnostic function in clang (https://github.com/llvm/llvm-project/blob/7240008c0afa3e2d12f3f51cfe0235668feb6ef3/clang/lib/Frontend/TextDiagnosticPrinter.cpp#L109) and flang (https://github.com/llvm/llvm-project/blob/7240008c0afa3e2d12f3f51cfe0235668feb6ef3/flang/lib/Frontend/TextDiagnosticPrinter.cpp#L32).
In particular, the passName is printed in the printDiagnosticOptions function https://github.com/llvm/llvm-project/blob/7240008c0afa3e2d12f3f51cfe0235668feb6ef3/clang/lib/Frontend/TextDiagnosticPrinter.cpp#L85

clang/lib/Basic/DiagnosticIDs.cpp

796–797 ↗

(On Diff #546831)

Looks like an unrelated change.

clang/lib/CodeGen/CodeGenAction.cpp

772 ↗

(On Diff #546831)

Looks like an unrelated change.

[tschuett]

flang/include/flang/Frontend/CodeGenOptions.h

72

enum class?

[awarzynski]

Hey @victorkingi , thank you for working on this :)

There's quite a lot going on here and I am thinking that it might be good to split this into a few patches? Also, please note that Flang's driver, unlike Clang, uses MLIR's coding style (use camelCase instead of CamelCase).

A StandaloneBackendConsumer was implemented but the DiagnosticsEngine still doesn't print to console the remarks produced.

This could mean that it's not being deconstructed correctly. Or that it requires explicit flushing. Can you verify that it contains the expected remarks?

[victorkingi]

added extraction of absolute file path in TextDiagnosticPrinter

[victorkingi]

removed unnecessary code

[victorkingi]

code cleanup

[victorkingi]

In D156320#4560839, @awarzynski wrote:

Hey @victorkingi , thank you for working on this :)

There's quite a lot going on here and I am thinking that it might be good to split this into a few patches? Also, please note that Flang's driver, unlike Clang, uses MLIR's coding style (use camelCase instead of CamelCase).

A StandaloneBackendConsumer was implemented but the DiagnosticsEngine still doesn't print to console the remarks produced.

This could mean that it's not being deconstructed correctly. Or that it requires explicit flushing. Can you verify that it contains the expected remarks?

Hi @awarzynski thanks for the comments. Splitting it up makes sense, I will do that.

[victorkingi]

fixing failing tests

[victorkingi]

from CamelCase to camelCase variables

[victorkingi]

changed enum to enum class

[victorkingi]

In D156320#4559248, @kiranchandramohan wrote:

rpass flag now prints remarks when requested but does not display
the passName used, i.e [-Rpass=inline]

I think the location information is also not printed. Please check the difference in implementation of the TextDiagnosticPrinter::HandleDiagnostic function in clang (https://github.com/llvm/llvm-project/blob/7240008c0afa3e2d12f3f51cfe0235668feb6ef3/clang/lib/Frontend/TextDiagnosticPrinter.cpp#L109) and flang (https://github.com/llvm/llvm-project/blob/7240008c0afa3e2d12f3f51cfe0235668feb6ef3/flang/lib/Frontend/TextDiagnosticPrinter.cpp#L32).
In particular, the passName is printed in the printDiagnosticOptions function https://github.com/llvm/llvm-project/blob/7240008c0afa3e2d12f3f51cfe0235668feb6ef3/clang/lib/Frontend/TextDiagnosticPrinter.cpp#L85

Hi @kiranchandramohan location now gets printed as the absolute path. In certain situations, clang preserves the dots e.g. if the user provides "../my_location/my_file" it will print the exact same, while flang expands to absolute path. So that would be one difference.

[victorkingi]

split the patch into 2. This is the implementation patch

[awarzynski]

flang/lib/Frontend/CompilerInvocation.cpp

173–174

Could you add a test to exercise this diagnostic?

[victorkingi]

Added remark error test and color printing for remark errors

[victorkingi]

flang/lib/Frontend/CompilerInvocation.cpp

762

Apparently without forwarding the color option to the CompilerInvocation, flang doesn't print remark errors with color. Hence the need for this.
Also, a question, why do we have 2 instances of DiagnosticsEngine, one in CompilerInvocation and the other passed as an argument?

flang/test/Driver/optimization-remark.f90

18–19

Testing error produced given a bad regex

[victorkingi]

rebasing

[victorkingi]

Removed false argument to ProcessWarningOption function to allow warning printing

[victorkingi]

Added warning tests in optimization-remark.f90

[awarzynski]

Thanks for the updates, more comments inline. Also:

The remarks printed miss carets.

Why and can you share an example?

The parseOptimizationRemark, addDiagnosticArgs and printDiagnosticOptions functions created are identical to the ones used in Clang.

In which case we should seriously consider moving this code somewhere where it can be shared. If outside the scope of this change, please document in code that there's scope for re-use.

flang/include/flang/Frontend/CodeGenOptions.h

72–118

From what I can see, this has been borrowed almost verbatim from Clang: https://github.com/llvm/llvm-project/blob/3a100ea901ed79d6a06a5f018be2b4d3bbca51e8/clang/include/clang/Basic/CodeGenOptions.h#L331-L376.

This is fine (and common throughout the driver), but please document more. In particular:

• Highlight that ATM this code is identical that what Clang contains (and add a TODO to perhaps share with Clang at some point),
• Highlight that the list of options in Flang and Clang is _identical - it is really good that the interfaces in Clang and Flang are consistent. That's a good reason to re-use the logic from Clang.

flang/lib/Frontend/CompilerInvocation.cpp

156–158

Could you give an example (and write a test ) for -Rgroup=regexp. Also, @kiranchandramohan , is this form actually needed? I am thinking that it's a complexity that could be avoided. It could definitely simplify this patch.

762

Apparently without forwarding the color option to the CompilerInvocation, flang doesn't print remark errors with color. Hence the need for this.

It is already "forwarded" here: https://github.com/llvm/llvm-project/blob/3a100ea901ed79d6a06a5f018be2b4d3bbca51e8/flang/lib/Frontend/CompilerInvocation.cpp#L117-L122. Could you explain why you need this change _here_?

Also, a question, why do we have 2 instances of DiagnosticsEngine, one in CompilerInvocation and the other passed as an argument?

One is for the driver itself, to generate diagnostics related to "driver errors" (e.g. option parsing errors). The other belongs to CompilerInstance rather than CompilerInvocation and is used to report compilation errors (e.g. semantic or parsing errors).

1023–1030

Is this specifically for parsing remarks options? Please add a comment

flang/lib/Frontend/FrontendActions.cpp

923

Why StandaloneBackendConsumer? Isn't this specifically for remarks? Also, could you document this class a bit?

flang/lib/Frontend/TextDiagnosticPrinter.cpp

37

Why is this method needed and can it be tested?

[kiranchandramohan]

flang/lib/Frontend/CompilerInvocation.cpp

156–158

Rpass=regex is used, particularly Rpass=.. So this is required, but can be deferred to a separate patch to simplify this one.

[awarzynski]

flang/lib/Frontend/CompilerInvocation.cpp

156–158

can be deferred to a separate patch to simplify this one

That would be a small win - the complexity comes from the fact that we can't rely on TableGen to define all possible options.

[victorkingi]

addressing comments with code refactoring

[awarzynski]

Thanks for trimming this, it's much easier to review! A few more suggestions, but nothing major.

flang/lib/Frontend/CompilerInvocation.cpp

236–266

// coment
if (const llvm::opt::Arg *a =
        args.getLastArg(clang::driver::options::OPT_opt_record_passes))
  opts.OptRecordPasses = a->getValue();

// coment
if (const llvm::opt::Arg *a =
        args.getLastArg(clang::driver::options::OPT_opt_record_format))
  opts.OptRecordFormat = a->getValue();

// coment
opts.OptimizationRemark = parseOptimizationRemark(
    diags, args, clang::driver::options::OPT_Rpass_EQ, "pass");

// coment
opts.OptimizationRemarkMissed = parseOptimizationRemark(
    diags, args, clang::driver::options::OPT_Rpass_missed_EQ, "pass-missed");

// coment
opts.OptimizationRemarkAnalysis = parseOptimizationRemark(
    diags, args, clang::driver::options::OPT_Rpass_analysis_EQ,
    "pass-analysis");

if (opts.getDebugInfo() == llvm::codegenoptions::NoDebugInfo) {
  // If the user requested a flag that requires source locations available in
  // the backend, make sure that the backend tracks source location information.
  bool needLocTracking = !opts.OptRecordFile.empty() || opts.OptRecordPasses ||
                         !opts.OptRecordFormat.empty() ||
                         opts.OptimizationRemark.hasValidPattern() ||
                         opts.OptimizationRemarkMissed.hasValidPattern() ||
                         opts.OptimizationRemarkAnalysis.hasValidPattern();

  if (needLocTracking)
    opts.setDebugInfo(llvm::codegenoptions::LocTrackingOnly);
}

I might have made typos when editing this, but hopefully the overall logic makes sense. Basically, I am suggesting that "option parsing" is separated from the logic for setting up the location tracking in the backend.

flang/lib/Frontend/FrontendActions.cpp

978–1005

https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code

void
optimizationRemarkHandler(const llvm::DiagnosticInfoOptimizationBase &d) {
  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.OptimizationRemark.patternMatches(d.getPassName()))
      emitOptimizationMessage(
          d, clang::diag::remark_fe_backend_optimization_remark);
    return;
  }

  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.OptimizationRemarkMissed.patternMatches(d.getPassName()))
      emitOptimizationMessage(
          d, clang::diag::remark_fe_backend_optimization_remark_missed);
    return;
  }

  assert(d.isAnalysis() && "Unknown remark type");

  bool shouldAlwaysPrint = false;
  if (auto *ora = llvm::dyn_cast<llvm::OptimizationRemarkAnalysis>(&d))
    shouldAlwaysPrint = ora->shouldAlwaysPrint();

  if (shouldAlwaysPrint ||
      codeGenOpts.OptimizationRemarkAnalysis.patternMatches(
          d.getPassName()))
    emitOptimizationMessage(
        d, clang::diag::remark_fe_backend_optimization_remark_analysis);
}

1007–1024

Where is this method used?

1143

https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

Variable names should be nouns (as they represent state). The name should be camel case, and start with an upper case letter (e.g. Leader or Boats).

flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp

225–227

Is this calling https://github.com/llvm/llvm-project/blob/c52d9509d40d3048914b144618232213e6076e05/clang/include/clang/Basic/Diagnostic.h#L1840-L1842? That's part of the clangBasic library. The overall goal in the driver is to reduce the dependency on Clang and this would be a step in the opposite direction. IIUC, we only need a small subset of that function, right?

[victorkingi]

code refactoring in reference to comments

[victorkingi]

added tests for "no" variants of Rpass

[victorkingi]

added comment

flang/lib/Frontend/CompilerInvocation.cpp

156–158

Removed need for Rpass=regex

173–174

added the tests in optimization-remark.f90 These are the -Rno-pass, -Rno-pass-analysis and -Rno-pass-missed tests.

762

Thanks for the explanation, A bad regex error would be printed without color. But since we are getting rid of the regex option, might as well remove this.

flang/lib/Frontend/FrontendActions.cpp

1007–1024

llvm/include/llvm/IR/DiagnosticHandler.h specifies that this method needs to be overridden if one wants to setup custom diagnostic format reporting. llvm/lib/IR/LLVMContext.cpp then uses it for reporting in the diagnose function

void LLVMContext::diagnose(const DiagnosticInfo &DI) {
  if (auto *OptDiagBase = dyn_cast<DiagnosticInfoOptimizationBase>(&DI))
    if (LLVMRemarkStreamer *RS = getLLVMRemarkStreamer())
      RS->emit(*OptDiagBase);

  // If there is a report handler, use it.
  if (pImpl->DiagHandler &&
      (!pImpl->RespectDiagnosticFilters || isDiagnosticEnabled(DI)) &&
      pImpl->DiagHandler->handleDiagnostics(DI))
    return;
  .
  .
  .

flang/lib/Frontend/TextDiagnosticPrinter.cpp

37

This method prints out the pass that was done e.g. [-Rpass=inline ], [-Rpass=loop-delete] next to the optimization message.
It is tested by the full remark message emitted test in flang/test/Driver/optimization-remark.f90

flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp

225–227

Yes, we only need a small subset. I'll create a static function in this file to avoid the dependence

[victorkingi]

flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp

225–227

Yes, we only need a small subset. I'll create a static function in this file to avoid the dependence

I mean normal function not static

[victorkingi]

flang/lib/Frontend/FrontendActions.cpp

923

I've added a comment, does this give a good explanation of what it does?

[awarzynski]

Thanks for the updates - this is looking really good! A few more suggestions and then I'll scan the whole thing again (sorry, there's been quite a lot of code going back and forth).

flang/lib/Frontend/CompilerInvocation.cpp

1024

flang/lib/Frontend/FrontendActions.cpp

976–1011

flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp

117

?

flang/test/Driver/optimization-remark.f90

53

FIXME

[awarzynski]

Some high level comments:

• The logic in parseCodeGenArgs in CompilerInvocation.cpp is a bit complex and quite specialized - could you move it to a dedicated method?
• In a fair few places this patch make references to "diagnostic flags" or "diagnostic options". That's borrowed from Clang where there's one code path for -W<opt> and -R<opt> flags. Note that in Clang the logic for -W<opt> is vastly more complex. This is completely absent in Flang. So, everything that's added here is added specifically for "remarks" and it would be helpful to be explicit about that. For example, processWarningOptions is misleading. processRemakrsOptions would be more accurate.

Thank you :)

flang/lib/Frontend/CompilerInvocation.cpp

227

// Specifies what passes to include.

Could you be more specific, what passes are you referring to? Included where?

242

// Specify which missed passes should be reported using a regex.

Reported where?

flang/lib/Frontend/TextDiagnosticPrinter.cpp

37

Flang has very _very_ limited support for warning flags. So this is going to be used specifically for Remarks. Please update and document accordingly.

flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp

30

No longer needed, right? Also, please use the the format consistent with the other #includes.

117

I find this method quite confusing.

1. It doesn't process warning options - it processes remarks options (so the name is inaccurate).
2. It deals with -Rpass -Rno-pass cases (i.e. postive + negative flag), but that's normally done in CompilerInvocation when parsing other options. See e.g. https://github.com/llvm/llvm-project/blob/c52d9509d40d3048914b144618232213e6076e05/flang/lib/Frontend/CompilerInvocation.cpp#L161-L163. Why not use that logic instead?
3. It's been extracted from Clang - it would be very helpful to note that and highlight the difference between this method and similar method in Clang.
4. You can safely trim it to only deal with Remarks (so e.g. update const clang::DiagnosticOptions &opts, in the signature)

Now, I am not requesting any major refactor here - I appreciate that e.g. these remark flags are defined a bit differently to other flags. But this method can be simplified and should be documented.

[victorkingi]

code refactoring

[victorkingi]

code refactoring

[victorkingi]

code refactoring

[victorkingi]

flang/lib/Frontend/FrontendActions.cpp

976–1011

The if statement still needs to return if the pattern doesn't match, should I leave it the way it is?

flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp

30

It's needed in the case of clang::diag::warn_unknown_diag_option in emitUnknownDiagWarning function.

117

I found it difficult including it in CompilerInvocation.cpp parseCodeGenArgs function since we are updating the DiagnosticsEngine object belonging to CompilerInstance not the other. We would have to expose the DiagnosticsEngine in CompilerInvocation class to do this. Would there be another way?

[victorkingi]

rebasing

[victorkingi]

rebasing

[awarzynski]

hey @victorkingi , I am still unsure about parsing these remarks options in two places:

• CompilerInvocation.cpp
• ExecuteCompilerInvocation.cpp

I think that it is important to clarify the relations between the two. In particular, it's normally the job of CompilerInvocaiton to make sure that e.g. -Rpass -Rno-pass -Rpass == -Rpass (so that DiagnosticOptions::Remarks only contains -Rpass). This might be tricky in practice if we want to support Regex, but would be good to document when e.g. populating DiagnosticOptions::Remarks.

I am also under the impression that extra complexity comes from the fact that this patch strives to support -R<some-random-thing-here> on top of -R{no}pass, -R{no}pass-missed, -R{no}pass-analysis. I also see some code left to support regex versions of the flags. Can you clean that up?

flang/include/flang/Frontend/CodeGenOptions.h

76–81

I only see RK_Enabled and RK_Disabled being used, though I don't see -Rgroup nor -Rno-group being tested 🤔 .

flang/lib/Frontend/CompilerInvocation.cpp

227

Do you know whether that only includes middle-end, or also back-end passes?

239

optimizationRemarkHandler is a member method of DiagnosticHandler, that you specialise in FrontendActions.cpp, right?

1024–1027

How about:

Preserve all the remark options requested, e.g. -Rpass, -Rpass-missed or -Rpass-analysis. This will be used later when processing and outputting the remarks generated by LLVM in  ExecuteCompilerInvocation.cpp.

flang/lib/Frontend/FrontendActions.cpp

976–1011

Sorry, my bad, I missed that. Yeah, then leave it as is, but could you replace const llvm::DiagnosticInfoOptimizationBase &d with something with more descriptive name? (I am referring to d)

flang/lib/Frontend/TextDiagnosticPrinter.cpp

18

WOuld you be able to use https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/Support/Path.h instead?

20

Would you be able to use https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/ADT/SmallVector.h instead?

20

Is this needed?

flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp

117

Thanks for the explanation, this helps understand the wider context. So it sounds like:

• this method has 2 important function: process the remark options _and_ update the DiagnosticEngine that belongs to this CompilerInstance
• the logic in CompilerInvocation does some initial option parsing to configure the code-gen (that's e.g. used by BackendRemarkConsumer)

Is this correct? I am also suggesting some edits below.

122–146

I am suggesting a few changes here. Some simplifications, some more descriptive variable names. Most importantly, updated function name - IIUC the key purpose of this function is to update the input DiagnosticsEngine based on -R... options. IMHO, the name should reflect that.

static void updateDiagEngineForOptRemarks(clang::DiagnosticsEngine &diagsEng,
                                 const clang::DiagnosticOptions &opts) {
  llvm::SmallVector<clang::diag::kind, 10> diags;
  const llvm::IntrusiveRefCntPtr<clang::DiagnosticIDs> diagIDs =
      diagsEng.getDiagnosticIDs();

  for (unsigned i = 0; i < opts.Remarks.size(); i++) {
    llvm::StringRef remarkOpt = opts.Remarks[i];
    const auto flavor = clang::diag::Flavor::Remark;

    // Check to see if this opt starts with "no-", if so, this is a
    // negative form of the option.
    bool isPositive = !remarkOpt.startswith("no-");
    if (!isPositive)
      remarkOpt = opt.substr(3);

    // Just issue a warning when encountering an unrecognised remark option.
    if (diagIDs->getDiagnosticsInGroup(flavor, remakrOpt, diags)) {
      emitUnknownDiagWarning(diagsEng, flavor, isPositive ? "-R" : "-Rno-", opt);
      continue; 
    }
    
    diags.setSeverityForGroup(flavor, remarkOpt,
                                isPositive ? clang::diag::Severity::Remark
                                           : clang::diag::Severity::Ignored);
  }
}

flang/test/Driver/optimization-remark.f90

8

How about something like this:

! RUN: %flang_fc1 %s -O1 -Runsupported_remark_opt -Rpass -emit-llvm -o - 2>&1 | FileCheck %s

[tschuett]

flang/lib/Frontend/CompilerInvocation.cpp

227

I use -Rpass-missed='gisel*' for GlobalIsel aka backend. I am interested in doing that exercise with Flang.

[victorkingi]

code refactoring

[victorkingi]

code refactoring

[victorkingi]

flang/include/flang/Frontend/CodeGenOptions.h

76–81

-Rgroup represents -Rpass, -Rpass-missed and -Rpass-analysis. Same applies to the no variation

flang/lib/Frontend/CompilerInvocation.cpp

227

Includes both middle and backend

239

No, it's just a member method of BackendRemarkConsumer

flang/lib/Frontend/TextDiagnosticPrinter.cpp

20

No it's not, I've removed it

[tschuett]

flang/lib/Frontend/CompilerInvocation.cpp

227

No. There are no middle-end passes that match that pattern.

[victorkingi]

flang/test/Driver/optimization-remark.f90

8

We are testing here if -Rno-pass is provided after -Rpass, nothing should be printed.

Unsupported value is tested at

! Check "unknown remark option" warning with suggestion
! RUN: %flang %s -O1 -Rpas 2>&1 | FileCheck %s --check-prefix=CHECK-WARN-SUGGEST

[victorkingi]

added support for backend remarks

[victorkingi]

flang/lib/Frontend/FrontendActions.cpp

1031–1042

This cases should handle back-end passes as the previous cases only handled middle-end

[tschuett]

Thanks!

[awarzynski]

Victor, this is proving quite tricky to review. There's already been a lot of updates and many of them are summarized as either "code refactor" or "clean-up". Please reduce traffic/noise and use more descriptive summaries.

Also, rather than adding new features in this already large change (I am referring to e.g. DK_MachineOptimizationRemarkMissed), please try to identify ways to split this patch further. Here are some suggestions (I've also made comments inline):

1. In the first iteration (like you effectively do now), focus on OPT_R_Joined options (e.g. -Rpass, Rpass-analysis, -Rpass-missed). Focus on basic functionality that demonstrates that correct information is returned from the backend. No need to fine tune the remarks with e.g. full file path or relevant remark option.
2. Next, add support for -Rpass=/-Rpass-missed=/-Rpass-analysis=. That's when e.g. llvm::opt::OptSpecifier optEq in parseOptimizationRemark would be needed (but not in Step 1).
3. Fine tune how the report is printed (e.g. improve file name by adding full path, add valid remark option at the end etc).
4. Add support for machine optimisations, e.g. DK_MachineOptimizationRemarkMissed.

This is easily 4 patches ;-)

flang/lib/Frontend/CompilerInvocation.cpp

158

optEq is not used, but it should be.

243–245

This is for -Rpass=, which is not tested. And the comment is inaccurate.

249–251

This is for -Rpass-missed=, which is not tested. And the comment is inaccurate.

255–256

This is for -Rpass-analysis=, which is not tested. And the comment is inaccurate.

flang/lib/Frontend/FrontendActions.cpp

1031–1042

Please move this to a dedicated patch with a test for each of these cases.

flang/lib/Frontend/TextDiagnosticPrinter.cpp

19

https://llvm.org/docs/CodingStandards.html#include-style

1. Main Module Header
2. Local/Private Headers
3. LLVM project/subproject headers (clang/..., lldb/..., llvm/..., etc)
4. System #includes

38–53

Move to a dedicated patch.

73–102

Move to a dedicated patch.

[victorkingi]

In D156320#4594584, @awarzynski wrote:

Victor, this is proving quite tricky to review. There's already been a lot of updates and many of them are summarized as either "code refactor" or "clean-up". Please reduce traffic/noise and use more descriptive summaries.

Also, rather than adding new features in this already large change (I am referring to e.g. DK_MachineOptimizationRemarkMissed), please try to identify ways to split this patch further. Here are some suggestions (I've also made comments inline):

1. In the first iteration (like you effectively do now), focus on OPT_R_Joined options (e.g. -Rpass, Rpass-analysis, -Rpass-missed). Focus on basic functionality that demonstrates that correct information is returned from the backend. No need to fine tune the remarks with e.g. full file path or relevant remark option.
2. Next, add support for -Rpass=/-Rpass-missed=/-Rpass-analysis=. That's when e.g. llvm::opt::OptSpecifier optEq in parseOptimizationRemark would be needed (but not in Step 1).
3. Fine tune how the report is printed (e.g. improve file name by adding full path, add valid remark option at the end etc).
4. Add support for machine optimisations, e.g. DK_MachineOptimizationRemarkMissed.

This is easily 4 patches ;-)

Hi Andrzej, splitting into 4 patches seems like a good idea. Here's the first patch that only handles the backend implementation https://reviews.llvm.org/D158174

[victorkingi]

Replaced by a series of patches already merged on github, starting from https://reviews.llvm.org/D157410

flang/include/flang/Frontend/CodeGenOptions.h

public:
 
   /// The regex that filters the passes that should be saved to the optimization
   /// records.
   std::string OptRecordPasses;
 
   /// The format used for serializing remarks (default: YAML)
   std::string OptRecordFormat;
 
+  // The RemarkKind enum class and OptRemark struct are identical to what Clang
+  // has
+  // TODO: Share with clang instead of re-implementing here
+  enum class 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 = RemarkKind::RK_Missing;
+    std::string Pattern;
+    std::shared_ptr<llvm::Regex> Regex;
+
+    /// By default, optimization remark is missing.
+    OptRemark() = default;
+
+    /// 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(llvm::StringRef string) const {
+      return hasValidPattern() && Regex->match(string);
+    }
+  };
+
+  // The OptRemark fields provided here are identical to Clang.
+
+  /// 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;
+
   // Define accessors/mutators for code generation options of enumeration type.
 #define CODEGENOPT(Name, Bits, Default)
 #define ENUM_CODEGENOPT(Name, Type, Bits, Default)                             \
   Type get##Name() const { return static_cast<Type>(Name); }                   \
   void set##Name(Type Value) { Name = static_cast<unsigned>(Value); }
 #include "flang/Frontend/CodeGenOptions.def"
 
   CodeGenOptions();
 };
 
 } // end namespace Fortran::frontend
 
 #endif

flang/lib/Frontend/CompilerInvocation.cpp

if (val != llvm::codegenoptions::DebugLineTablesOnly &&
       const auto debugWarning = diags.getCustomDiagID(
           clang::DiagnosticsEngine::Warning, "Unsupported debug option: %0");
       diags.Report(debugWarning) << arg->getValue();
     }
   }
   return true;
 }
 
+static CodeGenOptions::OptRemark
+parseOptimizationRemark(clang::DiagnosticsEngine &diags,
+                        llvm::opt::ArgList &args, llvm::opt::OptSpecifier optEq,
+                        llvm::StringRef name) {
+  CodeGenOptions::OptRemark result;
+
+  for (llvm::opt::Arg *a : args) {
+    if (a->getOption().matches(clang::driver::options::OPT_R_Joined)) {
+      llvm::StringRef value = a->getValue();
+
+      if (value == name)
+        result.Kind = CodeGenOptions::RemarkKind::RK_Enabled;
+      else if (value.split('-') == std::make_pair(llvm::StringRef("no"), name))
+        result.Kind = CodeGenOptions::RemarkKind::RK_Disabled;
+      else
+        continue;
+
+      if (result.Kind == CodeGenOptions::RemarkKind::RK_Disabled) {
+        result.Pattern = "";
+        result.Regex = nullptr;
+      } else {
+        result.Pattern = ".*";
+        result.Regex = std::make_shared<llvm::Regex>(result.Pattern);
+      }
+    }
+  }
+
+  return result;
+}
+
 static void parseCodeGenArgs(Fortran::frontend::CodeGenOptions &opts,
                              llvm::opt::ArgList &args,
                              clang::DiagnosticsEngine &diags) {
   opts.OptimizationLevel = getOptimizationLevel(args, diags);
 
   if (args.hasFlag(clang::driver::options::OPT_fdebug_pass_manager,
                    clang::driver::options::OPT_fno_debug_pass_manager, false))
     opts.DebugPassManager = 1;
else
       opts.PrepareForThinLTO = true;
   }
 
   // -f[no-]save-optimization-record[=<format>]
   if (const llvm::opt::Arg *a =
           args.getLastArg(clang::driver::options::OPT_opt_record_file))
     opts.OptRecordFile = a->getValue();
 
+  // Specifies what passes to include. If not provided
+  // -fsave-optimization-record will include all passes.
+  if (const llvm::opt::Arg *a =
+          args.getLastArg(clang::driver::options::OPT_opt_record_passes))
+    opts.OptRecordPasses = a->getValue();
+
+  // Optimization file format. Defaults to yaml
   if (const llvm::opt::Arg *a =
           args.getLastArg(clang::driver::options::OPT_opt_record_format))
     opts.OptRecordFormat = a->getValue();
 
-  if (const llvm::opt::Arg *a =
-          args.getLastArg(clang::driver::options::OPT_opt_record_passes))
-    opts.OptRecordPasses = a->getValue();
+  // Specify which successful passes should be reported using a regex.
+  opts.OptimizationRemark = parseOptimizationRemark(
+      diags, args, clang::driver::options::OPT_Rpass_EQ, "pass");
+
+  // Specify which missed passes should be reported using a regex.
+  opts.OptimizationRemarkMissed = parseOptimizationRemark(
+      diags, args, clang::driver::options::OPT_Rpass_missed_EQ, "pass-missed");
+
+  // Specify which passes, with additional information,
+  // should be reported using a regex.
+  opts.OptimizationRemarkAnalysis = parseOptimizationRemark(
+      diags, args, clang::driver::options::OPT_Rpass_analysis_EQ,
+      "pass-analysis");
+
+  if (opts.getDebugInfo() == llvm::codegenoptions::NoDebugInfo) {
+    // If the user requested a flag that requires source locations available in
+    // the backend, make sure that the backend tracks source location
+    // information.
+    bool needLocTracking = !opts.OptRecordFile.empty() ||
+                           !opts.OptRecordPasses.empty() ||
+                           !opts.OptRecordFormat.empty() ||
+                           opts.OptimizationRemark.hasValidPattern() ||
+                           opts.OptimizationRemarkMissed.hasValidPattern() ||
+                           opts.OptimizationRemarkAnalysis.hasValidPattern();
+
+    if (needLocTracking)
+      opts.setDebugInfo(llvm::codegenoptions::LocTrackingOnly);
+  }
 
   if (auto *a = args.getLastArg(clang::driver::options::OPT_save_temps_EQ))
     opts.SaveTempsDir = a->getValue();
 
   // -mrelocation-model option.
   if (const llvm::opt::Arg *a =
           args.getLastArg(clang::driver::options::OPT_mrelocation_model)) {
     llvm::StringRef modelName = a->getValue();
     auto relocModel =
for (const auto &wArg : wArgs) {
         diags.Report(diagID);
       }
     }
   }
 
   // Default to off for `flang-new -fc1`.
   res.getFrontendOpts().showColors =
       parseShowColorsArgs(args, /*defaultDiagColor=*/false);
 
   return diags.getNumErrors() == numErrorsBefore;
 }
 
 /// Parses all Dialect related arguments and populates the variables
 /// options accordingly. Returns false if new errors are generated.
 static bool parseDialectArgs(CompilerInvocation &res, llvm::opt::ArgList &args,
                              clang::DiagnosticsEngine &diags) {
   unsigned numErrorsBefore = diags.getNumErrors();
bool CompilerInvocation::createFromArgs(
   if (args.hasArg(clang::driver::options::OPT_flang_experimental_polymorphism)) {
     res.loweringOpts.setPolymorphicTypeImpl(true);
   }
 
   // -fno-ppc-native-vector-element-order
   if (args.hasArg(clang::driver::options::OPT_fno_ppc_native_vec_elem_order)) {
     res.loweringOpts.setNoPPCNativeVecElemOrder(true);
   }
 
+  // Add the remark option requested i.e. pass, pass-missed or pass-analysis.
+  // This will be used later during processing warnings and remarks to give
+  // messages specific to a remark argument. That happens in
+  // processWarningOptions in ExecuteCompilerInvocation.cpp
+  for (auto *a : args.filtered(clang::driver::options::OPT_R_Group))
+    res.getDiagnosticOpts().Remarks.push_back(a->getValue());
+
   success &= parseFrontendArgs(res.getFrontendOpts(), args, diags);
   parseTargetArgs(res.getTargetOpts(), args);
   parsePreprocessorArgs(res.getPreprocessorOpts(), args);
   parseCodeGenArgs(res.getCodeGenOpts(), args, diags);
   success &= parseDebugArgs(res.getCodeGenOpts(), args, diags);
   success &= parseSemaArgs(res, args, diags);
   success &= parseDialectArgs(res, args, diags);
   success &= parseDiagArgs(res, args, diags);

flang/lib/Frontend/FrontendActions.cpp

 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticFrontend.h"
 #include "clang/Driver/DriverDiagnostic.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Analysis/TargetLibraryInfo.h"
 #include "llvm/Analysis/TargetTransformInfo.h"
 #include "llvm/Bitcode/BitcodeWriterPass.h"
+#include "llvm/IR/DiagnosticPrinter.h"
 #include "llvm/IR/LLVMRemarkStreamer.h"
 #include "llvm/IR/LegacyPassManager.h"
 #include "llvm/IR/Verifier.h"
 #include "llvm/IRReader/IRReader.h"
 #include "llvm/MC/TargetRegistry.h"
 #include "llvm/Object/OffloadBinary.h"
 #include "llvm/Passes/PassBuilder.h"
 #include "llvm/Passes/PassPlugin.h"
#include "llvm/Support/Extension.def"
 
   if (action == BackendActionTy::Backend_EmitBC)
     mpm.addPass(llvm::BitcodeWriterPass(os));
 
   // Run the passes.
   mpm.run(*llvmModule, mam);
 }
 
+// This class handles optimization remark messages requested if
+// any of -Rpass, -Rpass-analysis or -Rpass-missed flags were provided
+class BackendRemarkConsumer : public llvm::DiagnosticHandler {
+
+  const CodeGenOptions &codeGenOpts;
+  clang::DiagnosticsEngine &diags;
+
+public:
+  BackendRemarkConsumer(clang::DiagnosticsEngine &diags,
+                        const CodeGenOptions &codeGenOpts)
+      : codeGenOpts(codeGenOpts), diags(diags) {}
+
+  bool isAnalysisRemarkEnabled(llvm::StringRef passName) const override {
+    return codeGenOpts.OptimizationRemarkAnalysis.patternMatches(passName);
+  }
+  bool isMissedOptRemarkEnabled(llvm::StringRef passName) const override {
+    return codeGenOpts.OptimizationRemarkMissed.patternMatches(passName);
+  }
+  bool isPassedOptRemarkEnabled(llvm::StringRef passName) const override {
+    return codeGenOpts.OptimizationRemark.patternMatches(passName);
+  }
+
+  bool isAnyRemarkEnabled() const override {
+    return codeGenOpts.OptimizationRemarkAnalysis.hasValidPattern() ||
+           codeGenOpts.OptimizationRemarkMissed.hasValidPattern() ||
+           codeGenOpts.OptimizationRemark.hasValidPattern();
+  }
+
+  void emitOptimizationMessage(const llvm::DiagnosticInfoOptimizationBase &d,
+                               unsigned diagID) {
+    // We only support warnings and remarks.
+    assert(d.getSeverity() == llvm::DS_Remark ||
+           d.getSeverity() == llvm::DS_Warning);
+
+    std::string msg;
+    llvm::raw_string_ostream msgStream(msg);
+
+    if (d.isLocationAvailable()) {
+      // Since sourceMgr isn't available, send file name and absolute path
+      // through msgStream, to use for printing
+      msgStream << d.getLocationStr() << ";;" << d.getAbsolutePath() << ";;";
+      msgStream << d.getMsg();
+    } else {
+      msgStream << d.getMsg();
+      llvm::DiagnosticPrinterRawOStream dp(msgStream);
+      d.print(dp);
+    }
+
+    // Emit message.
+    diags.Report(diagID) << clang::AddFlagValue(d.getPassName())
+                         << msgStream.str();
+  }
+
+  void
+  optimizationRemarkHandler(const llvm::DiagnosticInfoOptimizationBase &d) {
+    auto passName = d.getPassName();
+    if (d.isPassed() &&
+        codeGenOpts.OptimizationRemark.patternMatches(passName)) {
+      // Optimization remarks are active only if the -Rpass flag has a regular
+      // expression that matches the name of the pass name in \p d.
+      emitOptimizationMessage(
+          d, clang::diag::remark_fe_backend_optimization_remark);
+
+      return;
+    }
+
+    if (d.isMissed() &&
+        codeGenOpts.OptimizationRemarkMissed.patternMatches(passName)) {
+      // 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.
+      emitOptimizationMessage(
+          d, clang::diag::remark_fe_backend_optimization_remark_missed);
+
+      return;
+    }
+
+    assert(d.isAnalysis() && "Unknown remark type");
+
+    auto *ora = llvm::dyn_cast<llvm::OptimizationRemarkAnalysis>(&d);
+    if (!ora)
+      return;
+
+    bool shouldAlwaysPrint = ora->shouldAlwaysPrint();
+
+    if (shouldAlwaysPrint ||
+        codeGenOpts.OptimizationRemarkAnalysis.patternMatches(passName))
+      emitOptimizationMessage(
+          d, clang::diag::remark_fe_backend_optimization_remark_analysis);
+  }
+
+  bool handleDiagnostics(const llvm::DiagnosticInfo &di) override {
+    switch (di.getKind()) {
+    case llvm::DK_OptimizationRemark:
+      optimizationRemarkHandler(llvm::cast<llvm::OptimizationRemark>(di));
+      break;
+    case llvm::DK_OptimizationRemarkMissed:
+      optimizationRemarkHandler(llvm::cast<llvm::OptimizationRemarkMissed>(di));
+      break;
+    case llvm::DK_OptimizationRemarkAnalysis:
+      optimizationRemarkHandler(
+          llvm::cast<llvm::OptimizationRemarkAnalysis>(di));
+      break;
+    default:
+      break;
+    }
+    return true;
+  }
+};
+
 void CodeGenAction::embedOffloadObjects() {
   CompilerInstance &ci = this->getInstance();
   const auto &cgOpts = ci.getInvocation().getCodeGenOpts();
 
   for (llvm::StringRef offloadObject : cgOpts.OffloadObjects) {
     llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> objectOrErr =
         llvm::MemoryBuffer::getFileOrSTDIN(offloadObject);
     if (std::error_code ec = objectOrErr.getError()) {
       auto diagID = ci.getDiagnostics().getCustomDiagID(
           clang::DiagnosticsEngine::Error, "could not open '%0' for embedding");
       ci.getDiagnostics().Report(diagID) << offloadObject;
       return;
     }
     llvm::embedBufferInModule(
         *llvmModule, **objectOrErr, ".llvm.offloading",
         llvm::Align(llvm::object::OffloadBinary::getAlignment()));
   }
 }
void CodeGenAction::executeAction() {
   // an assert for incompatible data layout when the code-generation happens.
   llvmModule->setTargetTriple(theTriple);
   llvmModule->setDataLayout(tm->createDataLayout());
 
   // Embed offload objects specified with -fembed-offload-object
   if (!codeGenOpts.OffloadObjects.empty())
     embedOffloadObjects();
 
+  BackendRemarkConsumer remarkConsumer(diags, codeGenOpts);
+
+  llvmModule->getContext().setDiagnosticHandler(
+      std::make_unique<BackendRemarkConsumer>(remarkConsumer));
+
   // write optimization-record
   llvm::Expected<std::unique_ptr<llvm::ToolOutputFile>> optRecordFileOrErr =
       setupLLVMOptimizationRemarks(
           llvmModule->getContext(), codeGenOpts.OptRecordFile,
           codeGenOpts.OptRecordPasses, codeGenOpts.OptRecordFormat,
           /*DiagnosticsWithHotness=*/false,
           /*DiagnosticsHotnessThreshold=*/0);
 

flang/lib/Frontend/TextDiagnosticPrinter.cpp

 //
 //===----------------------------------------------------------------------===//
 //
 // Coding style: https://mlir.llvm.org/getting_started/DeveloperGuide/
 //
 //===----------------------------------------------------------------------===//
 
 #include "flang/Frontend/TextDiagnosticPrinter.h"
 #include "flang/Frontend/TextDiagnostic.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/raw_ostream.h"
+#include <clang/Frontend/TextDiagnosticPrinter.h>
+#include <filesystem>
+#include <string>
+#include <vector>
 
 using namespace Fortran::frontend;
 
 TextDiagnosticPrinter::TextDiagnosticPrinter(raw_ostream &diagOs,
                                              clang::DiagnosticOptions *diags)
     : os(diagOs), diagOpts(diags) {}
 
 TextDiagnosticPrinter::~TextDiagnosticPrinter() {}
 
+// Print any diagnostic option information to a raw_ostream.
+static void printDiagnosticOptions(llvm::raw_ostream &os,
+                                   clang::DiagnosticsEngine::Level level,
+                                   const clang::Diagnostic &info,
+                                   const clang::DiagnosticOptions &diagOpts) {
+  llvm::StringRef opt =
+      clang::DiagnosticIDs::getWarningOptionForDiag(info.getID());
+  if (!opt.empty()) {
+    os << " [" << (level == clang::DiagnosticsEngine::Remark ? "-R" : "-W")
+       << opt;
+    llvm::StringRef optValue = info.getDiags()->getFlagValue();
+    if (!optValue.empty())
+      os << "=" << optValue;
+    os << ']';
+  }
+}
+
 void TextDiagnosticPrinter::HandleDiagnostic(
     clang::DiagnosticsEngine::Level level, const clang::Diagnostic &info) {
   // Default implementation (Warnings/errors count).
   DiagnosticConsumer::HandleDiagnostic(level, info);
 
   // Render the diagnostic message into a temporary buffer eagerly. We'll use
   // this later as we print out the diagnostic to the terminal.
   llvm::SmallString<100> outStr;
   info.FormatDiagnostic(outStr);
 
   llvm::raw_svector_ostream diagMessageStream(outStr);
+  printDiagnosticOptions(diagMessageStream, level, info, *diagOpts);
 
   if (!prefix.empty())
     os << prefix << ": ";
 
   // We only emit diagnostics in contexts that lack valid source locations.
   assert(!info.getLocation().isValid() &&
          "Diagnostics with valid source location are not supported");
 
+  // split incoming string to get the absolute path and filename in the
+  // case we are receiving optimization remarks from BackendRemarkConsumer
+  std::string diagMsg = std::string(diagMessageStream.str());
+  std::string delimiter = ";;";
+
+  size_t pos = 0;
+  std::vector<std::string> tokens;
+  while ((pos = diagMsg.find(delimiter)) != std::string::npos) {
+    tokens.push_back(diagMsg.substr(0, pos));
+    diagMsg.erase(0, pos + delimiter.length());
+  }
+
+  // tokens will always be of size 2 in the case of optimization
+  // remark message received, in this format;
+  // [file location with line and column];;[path to file];;[the remark message]
+  if (tokens.size() == 2) {
+    // extract absolute path from the provided relative path
+    std::filesystem::path absPath(tokens[1]);
+    std::filesystem::path canonicalPath =
+        std::filesystem::weakly_canonical(absPath);
+
+    // we don't need the filename since we will append tokens[0]
+    // which is the filename, line and column number
+    canonicalPath.remove_filename();
+    absPath = canonicalPath.make_preferred().string();
+
+    // used for changing only the bold attribute
+    if (diagOpts->ShowColors)
+      os.changeColor(llvm::raw_ostream::SAVEDCOLOR, true);
+
+    // print absolute path, file name, line and column
+    os << absPath << tokens[0] << ": ";
+  }
+
   Fortran::frontend::TextDiagnostic::printDiagnosticLevel(os, level,
                                                           diagOpts->ShowColors);
   Fortran::frontend::TextDiagnostic::printDiagnosticMessage(
       os,
-      /*IsSupplemental=*/level == clang::DiagnosticsEngine::Note,
-      diagMessageStream.str(), diagOpts->ShowColors);
+      /*IsSupplemental=*/level == clang::DiagnosticsEngine::Note, diagMsg,
+      diagOpts->ShowColors);
 
   os.flush();
 }

flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp

 #include "mlir/IR/AsmState.h"
 #include "mlir/IR/MLIRContext.h"
 #include "mlir/Pass/PassManager.h"
 #include "clang/Driver/Options.h"
 #include "llvm/Option/OptTable.h"
 #include "llvm/Option/Option.h"
 #include "llvm/Support/BuryPointer.h"
 #include "llvm/Support/CommandLine.h"
+#include <clang/Basic/DiagnosticFrontend.h>
 
 namespace Fortran::frontend {
 
 static std::unique_ptr<FrontendAction>
 createFrontendAction(CompilerInstance &ci) {
 
   switch (ci.getFrontendOpts().programAction) {
   case InputOutputTest:
case PluginAction: {
     ci.getDiagnostics().Report(diagID) << ci.getFrontendOpts().actionName;
     return nullptr;
   }
   }
 
   llvm_unreachable("Invalid program action!");
 }
 
+// Emit a warning and typo hint for unknown warning opts
+static void emitUnknownDiagWarning(clang::DiagnosticsEngine &diags,
+                                   clang::diag::Flavor flavor,
+                                   llvm::StringRef prefix,
+                                   llvm::StringRef opt) {
+  llvm::StringRef suggestion =
+      clang::DiagnosticIDs::getNearestOption(flavor, opt);
+  diags.Report(clang::diag::warn_unknown_diag_option)
+      << (flavor == clang::diag::Flavor::WarningOrError ? 0 : 1)
+      << (prefix.str() += std::string(opt)) << !suggestion.empty()
+      << (prefix.str() += std::string(suggestion));
+}
+
+static void processWarningOptions(clang::DiagnosticsEngine &diags,
+                                  const clang::DiagnosticOptions &opts,
+                                  bool reportDiags = true) {
+
+  llvm::SmallVector<clang::diag::kind, 10> _diags;
+  const llvm::IntrusiveRefCntPtr<clang::DiagnosticIDs> diagIDs =
+      diags.getDiagnosticIDs();
+  // We parse the warning options twice.  The first pass sets diagnostic state,
+  // while the second pass reports warnings/errors.  This has the effect that
+  // we follow the more canonical "last option wins" paradigm when there are
+  // conflicting options.
+  for (unsigned report = 0, reportEnd = 2; report != reportEnd; ++report) {
+    bool setDiagnostic = (report == 0);
+
+    // If we've set the diagnostic state and are not reporting diagnostics then
+    // we're done.
+    if (!setDiagnostic && !reportDiags)
+      break;
+
+    for (unsigned i = 0, e = opts.Remarks.size(); i != e; ++i) {
+      llvm::StringRef opt = opts.Remarks[i];
+      const auto flavor = clang::diag::Flavor::Remark;
+
+      // Check to see if this opt starts with "no-", if so, this is a
+      // negative form of the option.
+      bool isPositive = !opt.startswith("no-");
+      if (!isPositive)
+        opt = opt.substr(3);
+
+      if (report) {
+        if (diagIDs->getDiagnosticsInGroup(flavor, opt, _diags))
+          emitUnknownDiagWarning(diags, flavor, isPositive ? "-R" : "-Rno-",
+                                 opt);
+      } else {
+        diags.setSeverityForGroup(flavor, opt,
+                                  isPositive ? clang::diag::Severity::Remark
+                                             : clang::diag::Severity::Ignored);
+      }
+    }
+  }
+}
+
 bool executeCompilerInvocation(CompilerInstance *flang) {
   // Honor -help.
   if (flang->getFrontendOpts().showHelp) {
     clang::driver::getDriverOptTable().printHelp(
         llvm::outs(), "flang-new -fc1 [options] file...",
         "LLVM 'Flang' Compiler",
         /*Include=*/clang::driver::options::FC1Option,
         /*Exclude=*/llvm::opt::DriverFlag::HelpHidden,
bool executeCompilerInvocation(CompilerInstance *flang) {
   // If there were errors in processing arguments, don't do anything else.
   if (flang->getDiagnostics().hasErrorOccurred()) {
     return false;
   }
 
   // Honor color diagnostics.
   flang->getDiagnosticOpts().ShowColors = flang->getFrontendOpts().showColors;
 
+  processWarningOptions(flang->getDiagnostics(), flang->getDiagnosticOpts());
+
   // Create and execute the frontend action.
   std::unique_ptr<FrontendAction> act(createFrontendAction(*flang));
   if (!act)
     return false;
 
   bool success = flang->executeAction(*act);
   return success;
 }
 
 } // namespace Fortran::frontend

flang/test/Driver/optimization-remark.f90

+! This file tests the -Rpass family of flags (-Rpass, -Rpass-missed
+! and -Rpass-analysis)
+! loop-delete isn't enabled at O0 so we use at least O1
+
+! Check that we can override -Rpass= with -Rno-pass.
+! RUN: %flang_fc1 %s -O1 -Rpass -emit-llvm -o - 2>&1 | FileCheck %s --check-prefix=CHECK-REMARKS
+! RUN: %flang_fc1 %s -O1 -Rpass -Rno-pass -emit-llvm -o - 2>&1 | FileCheck %s --check-prefix=CHECK-NO-REMARKS
+
+! Check "unknown remark option" warning
+! RUN: %flang %s -O1 -R 2>&1 | FileCheck %s --check-prefix=CHECK-REMARKS-WARN
+
+! Check "unknown remark option" warning with suggestion
+! RUN: %flang %s -O1 -Rpas 2>&1 | FileCheck %s --check-prefix=CHECK-WARN-SUGGEST
+
+! Check -Rno-pass, -Rno-pass-analysis, -Rno-pass-missed nothing emitted
+! RUN: %flang %s -O1 -Rno-pass 2>&1 | FileCheck %s --allow-empty --check-prefix=CHECK-NO-REMARKS
+! RUN: %flang %s -O1 -Rno-pass-missed 2>&1 | FileCheck %s --allow-empty --check-prefix=CHECK-NO-REMARKS
+! RUN: %flang %s -O1 -Rno-pass-analysis 2>&1 | FileCheck %s --allow-empty --check-prefix=CHECK-NO-REMARKS
+
+! Check full -Rpass message is emitted
+! RUN: %flang %s -O1 -Rpass 2>&1 | FileCheck %s
+
+! Check full -Rpass-missed message is emitted
+! RUN: %flang %s -O1 -Rpass-missed 2>&1 | FileCheck %s --check-prefix=CHECK-REMARKS-MISSED
+
+! Check full -Rpass-analysis message is emitted
+! RUN: %flang %s -O1 -Rpass-analysis 2>&1 | FileCheck %s --check-prefix=CHECK-REMARKS-ANALYSIS
+
+! CHECK: optimization-remark.f90:48:5: remark: Loop deleted because it is invariant [-Rpass=loop-delete]
+! CHECK-REMARKS-MISSED: optimization-remark.f90:43:5: remark: loop not vectorized [-Rpass-missed=loop-vectorize]
+! CHECK-REMARKS-ANALYSIS: optimization-remark.f90:43:5: remark: loop not vectorized: instruction cannot be vectorized [-Rpass-analysis=loop-vectorize]
+! CHECK-REMARKS: remark:
+! CHECK-NO-REMARKS-NOT: remark:
+
+! CHECK-REMARKS-WARN: warning: unknown remark option '-R' [-Wunknown-warning-option]
+! CHECK-WARN-SUGGEST: warning: unknown remark option '-Rpas'; did you mean '-Rpass'? [-Wunknown-warning-option]
+
+program forttest
+    implicit none
+    real, dimension(1:50) :: aR1
+    integer :: n
+
+    do n = 1,50
+        aR1(n) = n * 1.34
+        print *, "hello"
+    end do
+
+    do n = 1,50
+        aR1(n) = n * 1.34
+    end do
+
+end program forttest