victorkingi added inline comments.
================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:156-158
+/// Parse a remark command line argument. It may be missing, disabled/enabled
by
+/// '-R[no-]group' or specified with a regular expression by '-Rgroup=regexp'.
+/// On top of that, it can be disabled/enabled globally by '-R[no-]everything'.
----------------
awarzynski wrote:
> kiranchandramohan wrote:
> > awarzynski wrote:
> > > 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.
> > `Rpass=regex` is used, particularly `Rpass=.`. So this is required, but can
> > be deferred to a separate patch to simplify this one.
> > 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.
Removed need for Rpass=regex
================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:786
parseShowColorsArgs(args, /*defaultDiagColor=*/false);
+ res.getDiagnosticOpts().ShowColors = res.getFrontendOpts().showColors;
----------------
awarzynski wrote:
> victorkingi wrote:
> > 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?
> > 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?
>
> [[
> https://github.com/llvm/llvm-project/blob/3a100ea901ed79d6a06a5f018be2b4d3bbca51e8/flang/tools/flang-driver/fc1_main.cpp#L37-L40
> | One ]] is for the driver itself, to generate diagnostics related to
> "driver errors" (e.g. option parsing errors). The [[
> https://github.com/llvm/llvm-project/blob/3a100ea901ed79d6a06a5f018be2b4d3bbca51e8/flang/include/flang/Frontend/CompilerInstance.h#L64-L65
> | other ]] belongs to `CompilerInstance` rather than `CompilerInvocation`
> and is used to report compilation errors (e.g. semantic or parsing errors).
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.
================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:173-174
+ if (!result.Regex->isValid(regexError)) {
+ diags.Report(clang::diag::err_drv_optimization_remark_pattern)
+ << regexError << a->getAsString(args);
+ return false;
----------------
awarzynski wrote:
> Could you add a test to exercise this diagnostic?
added the tests in `optimization-remark.f90` These are the `-Rno-pass`,
`-Rno-pass-analysis` and `-Rno-pass-missed` tests.
================
Comment at: flang/lib/Frontend/FrontendActions.cpp:1007-1024
+ 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));
----------------
awarzynski wrote:
> Where is this method used?
`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;
.
.
.
```
================
Comment at: flang/lib/Frontend/TextDiagnosticPrinter.cpp:37
+// Print any diagnostic option information to a raw_ostream.
+static void printDiagnosticOptions(llvm::raw_ostream &os,
+ clang::DiagnosticsEngine::Level level,
----------------
awarzynski wrote:
> Why is this method needed and can it be tested?
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
================
Comment at: flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:169-171
+ clang::ProcessWarningOptions(flang->getDiagnostics(),
+ flang->getDiagnosticOpts());
+
----------------
awarzynski wrote:
> 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?
Yes, we only need a small subset. I'll create a static function in this file to
avoid the dependence
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
