This is an archive of the discontinued LLVM Phabricator instance.

[flang][driver] Add debug measure-parse-tree and pre-fir-tree options
ClosedPublic

Authored by FarisRehman on Feb 17 2021, 11:14 AM.

Details

Summary

Add the following options:

  • -fdebug-measure-parse-tree
  • -fdebug-pre-fir-tree

Summary of changes:

  • Add 2 new frontend actions: DebugMeasureParseTreeAction and DebugPreFIRTreeAction
  • Add MeasurementVisitor to FrontendActions.h
  • Make reportFatalSemanticErrors return true if there are any fatal errors
  • Port most of the -fdebug-pre-fir-tree tests to use the new driver if built, otherwise use f18

Diff Detail

Event Timeline

FarisRehman created this revision.Feb 17 2021, 11:14 AM
FarisRehman requested review of this revision.Feb 17 2021, 11:14 AM
Herald added a project: Restricted Project. · View Herald Transcript
FarisRehman added a project: Restricted Project.
FarisRehman edited the summary of this revision. (Show Details)Feb 17 2021, 11:22 AM

Rebase off main

Rebase off main and add a comment to MeasurementVisitor.

awarzynski accepted this revision.Feb 18 2021, 3:53 AM

Thank you for working on this @FarisRehman !

This semantics implemented here are identical to what f18 does. -fdebug-instrumented-parse from f18 becomes -fdebug-parsing-log in flang-new -fc1. This was proposed (and accepted) in the RFC.

I've left a few comments inline, but nothing major and otherwise this is ready to land.

flang/include/flang/Frontend/FrontendActions.h
18

This should be shared with f18.cpp, but currently we don't have a good place for that (and we probably want to understand how much is going to be shared first). So IMO it is fine to keep it here.

Could you add a comment similar to this so that it is clear that this should be moved at some point?

flang/lib/Frontend/FrontendActions.cpp
313

Could this use diagnostics instead? For example:

unsigned diagID = ci.diagnostics().getCustomDiagID(clang::DiagnosticsEngine::Error, "Pre FIR Tree is NULL.");
ci.diagnostics().Report(diagID);
flang/test/Flang-Driver/debug-measure-parse-tree.f90
24–25

The output for this test is:

Parse tree comprises 18 objects and occupies 1496 total bytes.

I think that the actual size (i.e. 18 and 1496) is not that relevant here and also prone to change (which may cause the test to become fragile).

Instead, I suggest that we verify that -fdebug-measure-parse-tree indeed instructs the driver to measure the parse and print the result. So, perhaps this instead:

Parse tree comprises {{[0-9]+}} objects and occupies {{[0-9]+}} total bytes.
flang/test/Flang-Driver/debug-parsing-log.f90
24

IIUC, this output is not really specific to -fdebug-parsing-log (-fsyntax-only prints this too). I think that this would be more specific to this flag:

! FRONTEN: fail 1
! FRONTEN: fail 2
! FRONTEN: fail 3

This is still not great, but looking at the log itself I struggle to notice anything that would stand out.

This revision is now accepted and ready to land.Feb 18 2021, 3:53 AM
awarzynski requested changes to this revision.Feb 18 2021, 3:53 AM
This revision now requires changes to proceed.Feb 18 2021, 3:53 AM

I've just realized that for -fdebug-parsing-log we need to set Fortran::parser::instrumentedParse. I don't see a good place for that just yet.

It's probably best extracting that option into a separate patch.

Remove -fdebug-parsing-log

Remove -fdebug-parsing-log from this patch as it requires additional work that can be implemented in a separate patch.
Also address review comments by @awarzynski

FarisRehman retitled this revision from [flang][driver] Add more -fdebug options to [flang][driver] Add debug measure-parse-tree and pre-fir-tree options.Feb 19 2021, 2:26 AM
FarisRehman edited the summary of this revision. (Show Details)
awarzynski accepted this revision.Feb 19 2021, 2:42 AM

Thank you for updating this @FarisRehman !

The original implementation of `-fdebug-parsing-log submitted here was not 100% compatible with -fdebug-instrumented-parse from f18 as we originally thought. I think that that's fine, but we need something that gives us such compatibility. We probably need another flag for that, e.g. -finstrumented-parse, that would toggle Fortran::parser::instrumentedParse. This way we will have this:

  • -fdebug-parsing-log (flang-new -fc1) != -fdebug-instrumented-parse (f18)
  • -fdebug-parsing-log -finstrumented-parse (flang-new -fc1) == -fdebug-instrumented-parse (f18)

But that's quite a few new scenarios and it's worth submitting as a separate patch.

You've also changed the order of flags in tests. This is also fine and guarantees that tests work with both f18 and flang-new -fc1:

  • in flang-new -fc1 only the last _action_ option is take into account, so -fsyntax-only is effectively ignored (and not needed)
  • in f18 we still need -fsyntax-only to make sure that code-generation is not run (e.g. to make f18 exit _after_ running the semantic checks), but the order is irrelevant

All this makes a lot of sense, thank you for adding these! LGTM!

This revision is now accepted and ready to land.Feb 19 2021, 2:42 AM
This revision was landed with ongoing or failed builds.Feb 19 2021, 3:35 AM
This revision was automatically updated to reflect the committed changes.