awarzynski added a comment. 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 <https://github.com/llvm/llvm-project/blob/c52d9509d40d3048914b144618232213e6076e05/clang/include/clang/Driver/Options.td#L2893-L2894> 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= <https://github.com/llvm/llvm-project/blob/c52d9509d40d3048914b144618232213e6076e05/clang/include/clang/Driver/Options.td#L2884-L2892>. 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 ;-) ================ Comment at: flang/lib/Frontend/CompilerInvocation.cpp:158 +parseOptimizationRemark(clang::DiagnosticsEngine &diags, + llvm::opt::ArgList &args, llvm::opt::OptSpecifier optEq, + llvm::StringRef name) { ---------------- `optEq` is not used, but it should be. ================ Comment at: flang/lib/Frontend/CompilerInvocation.cpp:244-246 + // Get -Rpass option regex. If empty, "".*"" is used. From all successful + // optimization passes applied, the regex will return only pass names that + // match it. ---------------- This is for `-Rpass=`, which is not tested. And the comment is inaccurate. ================ Comment at: flang/lib/Frontend/CompilerInvocation.cpp:250-252 + // Get -Rpass-missed option regex. If empty, "".*"" is used. From all + // optimization passes that failed to be applied, the regex will return only + // pass names that match it. ---------------- This is for `-Rpass-missed=`, which is not tested. And the comment is inaccurate. ================ Comment at: flang/lib/Frontend/CompilerInvocation.cpp:256-257 + + // Specify which passes, with additional information, + // should be reported using a regex. + opts.OptimizationRemarkAnalysis = parseOptimizationRemark( ---------------- This is for `-Rpass-analysis=`, which is not tested. And the comment is inaccurate. ================ Comment at: flang/lib/Frontend/FrontendActions.cpp:1032-1043 + case llvm::DK_MachineOptimizationRemark: + optimizationRemarkHandler( + llvm::cast<llvm::MachineOptimizationRemark>(di)); + break; + case llvm::DK_MachineOptimizationRemarkMissed: + optimizationRemarkHandler( + llvm::cast<llvm::MachineOptimizationRemarkMissed>(di)); ---------------- victorkingi wrote: > This cases should handle back-end passes as the previous cases only handled > middle-end Please move this to a dedicated patch with a test for each of these cases. ================ Comment at: flang/lib/Frontend/TextDiagnosticPrinter.cpp:19 #include "flang/Frontend/TextDiagnostic.h" +#include "string" #include "clang/Basic/DiagnosticOptions.h" ---------------- 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 ================ Comment at: flang/lib/Frontend/TextDiagnosticPrinter.cpp:39-54 +static void printRemarkOption(llvm::raw_ostream &os, + clang::DiagnosticsEngine::Level level, + const clang::Diagnostic &info) { + llvm::StringRef opt = + clang::DiagnosticIDs::getWarningOptionForDiag(info.getID()); + if (!opt.empty()) { + // We still need to check if the level is a Remark since, an unknown option ---------------- Move to a dedicated patch. ================ Comment at: flang/lib/Frontend/TextDiagnosticPrinter.cpp:76-105 + // 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; + llvm::SmallVector<std::string> tokens; ---------------- Move to a dedicated patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156320/new/ https://reviews.llvm.org/D156320 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
