This is an archive of the discontinued LLVM Phabricator instance.

[Flang][Driver] Implement OPT_R_Joined options
ClosedPublic

Authored by victorkingi on Aug 17 2023, 4:46 AM.

Details

Summary

Add a BackendRemarkConsumer class, responsible for handling diagnostics
received from LLVM. The diagnostics being information on middle and
backend passes used or not used.

Clang by default has all remarks ignored but manually sets the severity of
R_Group to visible(clang::diag::clang::Severity::Remark). This patch does
the same for Flang.

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

Diff Detail

Event Timeline

victorkingi created this revision.Aug 17 2023, 4:46 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
victorkingi requested review of this revision.Aug 17 2023, 4:46 AM
victorkingi updated this revision to Diff 551092.EditedAug 17 2023, 4:58 AM

In flang/lib/Frontend/FrontendActions.cpp removed
DiagnosticPrinter import, no longer needed.

victorkingi edited the summary of this revision. (Show Details)Aug 17 2023, 5:01 AM
awarzynski added inline comments.Aug 17 2023, 5:10 AM
flang/lib/Frontend/CompilerInvocation.cpp
229–234

Is this needed? I don't see any tests for -opt-record-passes.

249–260

IIUC, this is not needed in this patch.

victorkingi added inline comments.Aug 17 2023, 6:02 AM
flang/lib/Frontend/CompilerInvocation.cpp
229–234

This is present in main branch, it is not part of this patch, the reason it's marked green is because I moved its position just before

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

I'll move it back to its previous location to avoid the confusion

249–260

It is needed since we are implementing the 3 flags

To avoid green highlighting on code I did not edit, in
flang/lib/Frontend/CompilerInvocation.cpp, reordered if statement
handling fsave-optimization file record format and passes in line
224 and 231 back to what they were originally.

victorkingi marked 2 inline comments as done.Aug 17 2023, 6:12 AM
kiranchandramohan added inline comments.
flang/test/Driver/optimization-remark.f90
14

The warnings for unknown remarks is probably its own patch.

Thanks for extracting this, more comments inline.

flang/lib/Frontend/CompilerInvocation.cpp
156–158

Please document this method. Also, it looks like the list of valid values of name is limited? Can you add an assert for that?

158

llvm::opt::OptSpecifier optEq is not used

159

name of what? Please avoid such generic variable ... names :)

244–246

Regex is not yet supported. Same comment below.

victorkingi marked 5 inline comments as done.

Updated comments on opts.OptimizationRemark in line 235 in flang/lib/Frontend/CompilerInvocation.cpp
Removed warning handling in updateDiagEngineForOptRemark as well as the tests accompanying it.
Removed optEq argument from parseOptimizationRemark.

Documented the parseOptimizationRemark method
Renamed assertion message in line 163 in flang/lib/Frontend/CompilerInvocation.cpp

Thanks for all the updates, LG % some minor comments (please address before landing this). Great work!

[nit] You can safely drop this from the summary and focus on the contents of the patch itself:

It was created to address the comment about splitting into 4 patches in https://reviews.llvm.org/D156320#4594584

Here's a great intro on writing Git commit messages: https://cbea.ms/git-commit/#imperative

flang/lib/Frontend/CompilerInvocation.cpp
159–160

A "group" in Options.td has a bit special meaning. See e.g. R_Group. More specifically, -Rpass, -Rpass-analysis and -Rpass-missed belong to the R_Group group. So, instead of rGroupName, you probably meant something like remarkType or remarkTypeName or remarkOptName ;-)

172

[nit] Comment that this enables everything

178

[nit] Comment that this disables everything

flang/test/Driver/optimization-remark.f90
2

[nit] You can drop CHECK from custom prefixes. IMHO that's just noise.

25–26

Is there anything that could be checked with e.g. CHECK-REMARKS-MISSED-NOT and CHECK-REMARKS-ANALYSIS-NOT?

awarzynski accepted this revision.Aug 19 2023, 11:16 AM
This revision is now accepted and ready to land.Aug 19 2023, 11:16 AM
victorkingi edited the summary of this revision. (Show Details)Aug 21 2023, 2:08 AM
victorkingi marked 4 inline comments as done.

Removed unnecessary custom prefix CHECK in optimization-remark.f90 tests.
Added "disable and enable everything" comment to parseOptimizationRemark.

flang/test/Driver/optimization-remark.f90
25–26

Nothing comes to mind. Maybe checking if a successful pass was printed instead of a missed pass, but would that be redundant?

victorkingi edited the summary of this revision. (Show Details)Aug 21 2023, 3:07 AM
victorkingi edited the summary of this revision. (Show Details)Aug 21 2023, 3:10 AM

removed unused diags argument in parseOptimizationRemark.

awarzynski added inline comments.Aug 22 2023, 6:31 AM
flang/test/Driver/optimization-remark.f90
25–26

Output with -Rpass-missed:

remark: _FortranAioBeginExternalListOutput will not be inlined into _QQmain because its definition is unavailable
remark: _FortranAioOutputAscii will not be inlined into _QQmain because its definition is unavailable
remark: _FortranAioEndIoStatement will not be inlined into _QQmain because its definition is unavailable
remark: _FortranAioBeginExternalListOutput will not be inlined into _QQmain because its definition is unavailable
remark: _FortranAioOutputAscii will not be inlined into _QQmain because its definition is unavailable
remark: _FortranAioEndIoStatement will not be inlined into _QQmain because its definition is unavailable
remark: loop not vectorized

Output with -Rpass-analysis:

remark: loop not vectorized: call instruction cannot be vectorized
remark: loop not vectorized: instruction cannot be vectorized

So:

! CHECK-REMARKS-MISSED-NOT: instruction cannot be vectorized
! CHECK-REMARKS-ANALYSIS-NOT: will not be inlined into {{.*}} because its definition is unavailable

Wouldn't this work?

victorkingi added inline comments.Aug 23 2023, 2:14 AM
flang/test/Driver/optimization-remark.f90
25–26

I assumed flang might later add support for the missing definitions, causing the test to fail in the future. But now that I think of it, will cross the bridge when we get there :)

I'll update the patch to include that.

victorkingi added inline comments.Aug 23 2023, 2:36 AM
flang/test/Driver/optimization-remark.f90
25–26

I assumed flang might later add support for the missing definitions, causing the test to fail in the future. But now that I think of it, will cross the bridge when we get there :)

I'll update the patch to include that.

Ignore this comment, I misunderstood what you wanted me to do.

Added NOT test cases for REMARK-MISSED and REMARK-ANALYSIS in
optimization-remark.f90

victorkingi marked 2 inline comments as done.Aug 23 2023, 2:42 AM
awarzynski added inline comments.Aug 23 2023, 2:50 AM
flang/test/Driver/optimization-remark.f90
25–28

[nit] To make things easier for our future selves:

! REMARKS-MISSED-NOT: loop not vectorized: instruction cannot be vectorized
! REMARKS-MISSED: remark: loop not vectorized
! REMARKS-MISSED: {{.*}} will not be inlined into {{.*}} because its definition is unavailable

! REMARKS-ANALYSIS-NOT: {{.*}} will not be inlined into {{.*}} because its definition is unavailable
! REMARKS-ANALYSIS: remark: loop not vectorized: instruction cannot be vectorized

Does this make sense?

victorkingi added inline comments.Aug 23 2023, 5:21 AM
flang/test/Driver/optimization-remark.f90
25–28

yes it does. Since the patch landed, I've added as an NFC commit here https://reviews.llvm.org/D158599

victorkingi marked an inline comment as done.Aug 23 2023, 8:13 AM