[Flang][Driver] Add regex support for R_Group options

[victorkingi]

Summary

Add regex handling for all variations of OPT_R_Joined, i.e.
-Rpass, -Rpass-analysis, -Rpass-missed.

Depends on D158174. That patch implements backend support for
R_Group options.

[victorkingi]

flang/lib/Frontend/CompilerInvocation.cpp

766–768

For the bad regex error to be printed with colors, we would need to pass showColors from Frontend Options to Diagnostics Options much earlier on in CompilerInvocation::createFromArgs . Reason being regex errors from R family of flags don't get that far into the execution.

[awarzynski]

flang/lib/Frontend/CompilerInvocation.cpp

164

[nit] This is unrelated to this change, so just a nice-to-have

256

Just a nice-to-have

1035–1036

So, in -Rpass=<regex>you would only push -Rpass, right?

flang/test/Driver/optimization-remark.f90

18

At the moment this test does not differentiate between -Rpass=loop and -Rpass - in both cases the output is identical. Could you extend the test so that there's an actual difference?

[victorkingi]

Changed optimization-remark.f90 test code to generate more unique
missed and successful passes. -Rpass=loop and -Rpass should have
unique outputs hence, testable.
Minor comments added in CompilerInvocation.cpp

[victorkingi]

flang/lib/Frontend/CompilerInvocation.cpp

1035–1036

You will only push "pass". I've updated the comment to make it clearer.

[victorkingi]

Moved invalid regex test to optimization-remark-invalid.f90

[awarzynski]

Makes sense, a few minor suggestion. Also, could you rebase on top of main?

flang/test/Driver/optimization-remark.f90

15

[nit] IMHO, PASS-LOOP-ONLY would be more descriptive. Or even PASS-REGEX-LOOP-ONLY. "REGEX" refers to an implementation detail, but here we are more interested in the actual functionality.

35

Some comments explaining _what to expect_ would help. For example, with plain -Rpass we expect remarks related to 2 opportunities. Once we start filtering, this is reduced to 1, right?

39

Could you re-order the CHECK lines so that it matches the RUN lines? So:

! REMARKS:
! NO-REMARKS:

! PASS-REGEX-NOT:
! PASS-REGEX:

! MISSED-REGEX-NOT:
! MISSED-REGEX:

! ANALYSIS-REGEX-NOT:
! ANALYSIS-REGEX:

[victorkingi]

Reused same definition of the output in optimization-remark.f90 as well
as discarded the output generated.

Replaced '-c' flag references with '-S' to only handle pre-process and
compile steps in optimization-remark.f90.

flang/test/Driver/optimization-remark.f90

35

Yes

[awarzynski]

LGTM, thank you for addressing my comments!

flang/test/Driver/optimization-remark.f90

72–78

[victorkingi]

updated comment in optimization-remark.f90, attempt to fix failing test
case.

[victorkingi]

Attempt to fix failing optimization-remark.f90 test

[victorkingi]

[nit] optimization-remark.f90 line 80 'enddo' to 'end do'

[victorkingi]

no changes, rebasing

[victorkingi]

no changes, rebasing

flang/include/flang/Frontend/CodeGenOptions.h

public:
 
   /// 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', i.e. -Rpass, -Rpass-missed,
+    RK_Missing,     // Remark argument not present on the command line.
+    RK_Enabled,     // Remark enabled via '-Rgroup', i.e. -Rpass, -Rpass-missed,
                     // -Rpass-analysis
-    RK_Disabled, // Remark disabled via '-Rno-group', i.e. -Rno-pass,
+    RK_Disabled,    // Remark disabled via '-Rno-group', i.e. -Rno-pass,
                     // -Rno-pass-missed, -Rno-pass-analysis.
+    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;
 

flang/lib/Frontend/CompilerInvocation.cpp

if (llvm::opt::Arg *arg =
     }
   }
   return true;
 }
 
 // Generate an OptRemark object containing info on if the -Rgroup
 // specified is enabled or not.
 static CodeGenOptions::OptRemark
-parseOptimizationRemark(llvm::opt::ArgList &args,
+parseOptimizationRemark(clang::DiagnosticsEngine &diags,
+                        llvm::opt::ArgList &args, llvm::opt::OptSpecifier optEq,
                         llvm::StringRef remarkOptName) {
   assert((remarkOptName == "pass" || remarkOptName == "pass-missed" ||
           remarkOptName == "pass-analysis") &&
-         "Unknown group name provided.");
+         "Unsupported remark option name provided.");
   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 == remarkOptName) {
         result.Kind = CodeGenOptions::RemarkKind::RK_Enabled;
         // Enable everything
         result.Pattern = ".*";
         result.Regex = std::make_shared<llvm::Regex>(result.Pattern);
 
       } else if (value.split('-') ==
                  std::make_pair(llvm::StringRef("no"), remarkOptName)) {
         result.Kind = CodeGenOptions::RemarkKind::RK_Disabled;
         // Disable everything
         result.Pattern = "";
         result.Regex = nullptr;
       }
+    } else if (a->getOption().matches(optEq)) {
+      result.Kind = CodeGenOptions::RemarkKind::RK_WithPattern;
+      result.Pattern = a->getValue();
+      result.Regex = std::make_shared<llvm::Regex>(result.Pattern);
+      std::string regexError;
+
+      if (!result.Regex->isValid(regexError)) {
+        diags.Report(clang::diag::err_drv_optimization_remark_pattern)
+            << regexError << a->getAsString(args);
+        return CodeGenOptions::OptRemark();
+      }
     }
   }
   return result;
 }
 
 static void parseCodeGenArgs(Fortran::frontend::CodeGenOptions &opts,
                              llvm::opt::ArgList &args,
                              clang::DiagnosticsEngine &diags) {
static void parseCodeGenArgs(Fortran::frontend::CodeGenOptions &opts,
   // backend), to include in the final optimization record file generated. 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();
 
   // Create OptRemark that allows printing of all successful optimization
   // passes applied.
-  opts.OptimizationRemark = parseOptimizationRemark(args, "pass");
+  opts.OptimizationRemark =
+      parseOptimizationRemark(diags, args, clang::driver::options::OPT_Rpass_EQ,
+                              /*remarkOptName=*/"pass");
 
   // Create OptRemark that allows all missed optimization passes to be printed.
-  opts.OptimizationRemarkMissed = parseOptimizationRemark(args, "pass-missed");
+  opts.OptimizationRemarkMissed = parseOptimizationRemark(
+      diags, args, clang::driver::options::OPT_Rpass_missed_EQ,
+      /*remarkOptName=*/"pass-missed");
 
   // Create OptRemark that allows all optimization decisions made by LLVM
   // to be printed.
-  opts.OptimizationRemarkAnalysis =
-      parseOptimizationRemark(args, "pass-analysis");
+  opts.OptimizationRemarkAnalysis = parseOptimizationRemark(
+      diags, args, clang::driver::options::OPT_Rpass_analysis_EQ,
+      /*remarkOptName=*/"pass-analysis");
 
   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();
for (const auto &wArg : wArgs) {
       }
     }
   }
 
   // Default to off for `flang-new -fc1`.
   res.getFrontendOpts().showColors =
       parseShowColorsArgs(args, /*defaultDiagColor=*/false);
 
+  // Honor color diagnostics.
+  res.getDiagnosticOpts().ShowColors = res.getFrontendOpts().showColors;
+
   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(
   // -fno-ppc-native-vector-element-order
   if (args.hasArg(clang::driver::options::OPT_fno_ppc_native_vec_elem_order)) {
     res.loweringOpts.setNoPPCNativeVecElemOrder(true);
   }
 
   // Preserve all the remark options requested, i.e. -Rpass, -Rpass-missed or
   // -Rpass-analysis. This will be used later when processing and outputting the
   // remarks generated by LLVM in ExecuteCompilerInvocation.cpp.
-  for (auto *a : args.filtered(clang::driver::options::OPT_R_Group))
+  for (auto *a : args.filtered(clang::driver::options::OPT_R_Group)) {
+    if (a->getOption().matches(clang::driver::options::OPT_R_value_Group))
+      // This is -Rfoo=, where foo is the name of the diagnostic
+      // group. Add only the remark option name to the diagnostics. e.g. for
+      // -Rpass= we will add the string "pass".
+      res.getDiagnosticOpts().Remarks.push_back(
+          std::string(a->getOption().getName().drop_front(1).rtrim("=-")));
+    else
+      // If no regex was provided, add the provided value, e.g. for -Rpass add
+      // the string "pass".
       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);

flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp

if (!flang->getFrontendOpts().mlirArgs.empty()) {
     llvm::cl::ParseCommandLineOptions(numArgs + 1, args.get());
   }
 
   // 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;
-
   updateDiagEngineForOptRemarks(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=REMARKS
 ! RUN: %flang_fc1 %s -O1 -Rpass -Rno-pass -emit-llvm -o - 2>&1 | FileCheck %s --check-prefix=NO-REMARKS
 
 ! Check -Rno-pass, -Rno-pass-analysis, -Rno-pass-missed nothing emitted
 ! RUN: %flang %s -O1 -Rno-pass -c 2>&1 | FileCheck %s --allow-empty --check-prefix=NO-REMARKS
 ! RUN: %flang %s -O1 -Rno-pass-missed -c 2>&1 | FileCheck %s --allow-empty --check-prefix=NO-REMARKS
 ! RUN: %flang %s -O1 -Rno-pass-analysis -c 2>&1 | FileCheck %s --allow-empty --check-prefix=NO-REMARKS
 
+! Check error on invalid regex -Rpass message is emitted
+! RUN: not %flang %s -O1 -Rpass=[ -c 2>&1 | FileCheck %s --check-prefix=REGEX-INVALID
+
+! Check valid -Rpass regex
+! RUN: %flang %s -O1 -Rpass=loop -c 2>&1 | FileCheck %s --check-prefix=PASS-REGEX
+
+! Check valid -Rpass-missed regex
+! RUN: %flang %s -O1 -Rpass-missed=loop -c 2>&1 | FileCheck %s --check-prefix=MISSED-REGEX
+
+! Check valid -Rpass-analysis regex
+! RUN: %flang %s -O1 -Rpass-analysis=loop -c 2>&1 | FileCheck %s --check-prefix=ANALYSIS-REGEX
+
 ! Check full -Rpass message is emitted
-! RUN: %flang %s -O1 -Rpass -c 2>&1 | FileCheck %s
+! RUN: %flang %s -O1 -Rpass -c 2>&1 | FileCheck %s --check-prefix=PASS
 
 ! Check full -Rpass-missed message is emitted
-! RUN: %flang %s -O1 -Rpass-missed -c 2>&1 | FileCheck %s --check-prefix=REMARKS-MISSED
+! RUN: %flang %s -O1 -Rpass-missed -c 2>&1 | FileCheck %s --check-prefix=MISSED
 
 ! Check full -Rpass-analysis message is emitted
-! RUN: %flang %s -O1 -Rpass-analysis -c 2>&1 | FileCheck %s --check-prefix=REMARKS-ANALYSIS
+! RUN: %flang %s -O1 -Rpass-analysis -c 2>&1 | FileCheck %s --check-prefix=ANALYSIS
+
+
+! REGEX-INVALID:      error: in pattern '-Rpass=[': brackets ([ ]) not balanced
 
-! CHECK: remark: Loop deleted because it is invariant
-! REMARKS-MISSED: {{.*}} will not be inlined into {{.*}} because its definition is unavailable
-! REMARKS-MISSED: remark: loop not vectorized
-! REMARKS-MISSED-NOT: loop not vectorized: instruction cannot be vectorized
-! REMARKS-ANALYSIS: remark: loop not vectorized: instruction cannot be vectorized
-! REMARKS-ANALYSIS-NOT: {{.*}} will not be inlined into {{.*}} because its definition is unavailable
+! PASS:               remark: hoisting load
+! PASS:               remark: Loop deleted because it is invariant
+
+! PASS-REGEX-NOT:     remark: hoisting load
+! PASS-REGEX:         remark: Loop deleted because it is invariant
+
+! MISSED:             remark: failed to hoist load with loop-invariant address because load is conditionally executed
+! MISSED:             remark: loop not vectorized
+! MISSED-NOT:         remark: loop not vectorized: unsafe dependent memory operations in loop. Use #pragma loop distribute(enable) to allow loop distribution to attempt to isolate the offending operations into a separate loop
+!                     Unknown data dependence.
+
+! MISSED-REGEX-NOT:   remark: failed to hoist load with loop-invariant address because load is conditionally executed
+! MISSED-REGEX:       remark: loop not vectorized
+
+! ANALYSIS-REGEX:     remark: loop not vectorized: unsafe dependent memory operations in loop. Use #pragma loop distribute(enable) to allow loop distribution to attempt to isolate the offending operations into a separate loop
+!                     Unknown data dependence.
+! ANALYSIS-REGEX-NOT: remark: {{.*}}: IR instruction count changed from {{[0-9]+}} to {{[0-9]+}}; Delta: {{[0-9]+}}
+
+! ANALYSIS:           remark: loop not vectorized: unsafe dependent memory operations in loop. Use #pragma loop distribute(enable) to allow loop distribution to attempt to isolate the offending operations into a separate loop
+!                     Unknown data dependence.
+! ANALYSIS:           remark: {{.*}}: IR instruction count changed from {{[0-9]+}} to {{[0-9]+}}; Delta: {{[0-9]+}}
+! ANALYSIS-NOT:       remark: failed to hoist load with loop-invariant address because load is conditionally executed
 
 ! REMARKS: remark:
 ! NO-REMARKS-NOT: remark:
 
-program forttest
-    implicit none
-    real, dimension(1:50) :: aR1
-    integer :: n
+subroutine swap_real(a1, a2)
    
-    do n = 1,50
-        aR1(n) = n * 1.34
-        print *, "hello"
-    end do
+   real, dimension(1:2) :: aR1
+   integer :: i, n
+   real, intent(inout) :: a1(:), a2(:)
+   real :: a
+
+!  Swap
+   do i = 1, min(size(a1), size(a2))
+      a = a1(i)
+      a1(i) = a2(i)
+      a2(i) = a
+   enddo
 
-    do n = 1,50
+! Do a random loop to generate a successful loop-delete pass
+    do n = 1,2
         aR1(n) = n * 1.34
     end do
 
-end program forttest
+end subroutine swap_real