awarzynski added a comment.
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?
================
Comment at: flang/include/flang/Frontend/CodeGenOptions.h:76-81
+ 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'.
----------------
I only see `RK_Enabled` and `RK_Disabled` being used, though I don't see
`-Rgroup` nor `-Rno-group` being tested 🤔 .
================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:227
+ // Specifies, using a regex, which successful optimization passes done,
+ // to include in the final optimization record file generated. If not
provided
----------------
Do you know whether that only includes middle-end, or also back-end passes?
================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:240
+ // OptimizationRemark, OptimizationRemarkMissed and
OptimizationRemarkAnalysis
+ // contain regex values which are used in optimizationRemarkHandler in
+ // FrontendActions.cpp to determine which remarks generated should be
outputed
----------------
`optimizationRemarkHandler` is a member method of `DiagnosticHandler`, that you
specialise in FrontendActions.cpp, right?
================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:1034-1037
+ // 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
----------------
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.
```
================
Comment at: flang/lib/Frontend/FrontendActions.cpp:976-1011
+ 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(
----------------
victorkingi wrote:
> awarzynski wrote:
> >
> The if statement still needs to return if the pattern doesn't match, should I
> leave it the way it is?
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`)
================
Comment at: flang/lib/Frontend/TextDiagnosticPrinter.cpp:18
#include "flang/Frontend/TextDiagnosticPrinter.h"
+#include "filesystem"
#include "flang/Frontend/TextDiagnostic.h"
----------------
WOuld you be able to use
https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/Support/Path.h
instead?
================
Comment at: flang/lib/Frontend/TextDiagnosticPrinter.cpp:21
+#include "string"
+#include "vector"
#include "clang/Basic/DiagnosticOptions.h"
----------------
Would you be able to use
https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/ADT/SmallVector.h
instead?
================
Comment at: flang/lib/Frontend/TextDiagnosticPrinter.cpp:23
#include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Frontend/TextDiagnosticPrinter.h"
#include "llvm/ADT/SmallString.h"
----------------
Is this needed?
================
Comment at: flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:122-146
+static void processRemarkOptions(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();
+
----------------
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);
}
}
```
================
Comment at: flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:117
+
+void processWarningOptions(clang::DiagnosticsEngine &diags,
+ const clang::DiagnosticOptions &opts,
----------------
victorkingi wrote:
> awarzynski wrote:
> > awarzynski wrote:
> > > ?
> > 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.
> 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?
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.
================
Comment at: flang/test/Driver/optimization-remark.f90:7
+! 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
+
----------------
How about something like this:
```
! RUN: %flang_fc1 %s -O1 -Runsupported_remark_opt -Rpass -emit-llvm -o - 2>&1 |
FileCheck %s
```
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
