This is an archive of the discontinued LLVM Phabricator instance.

[flang][driver] Add support for `-emit-mlir`
ClosedPublic

Authored by awarzynski on Feb 4 2022, 3:26 AM.

Details

Summary

This patch adds support for generating MLIR files in Flang's frontend
driver (i.e. flang-new -fc1). -emit-fir is added as an alias for
-emit-mlir. We may want to decide to split the two in the future.

A new parent class for code-gen frontend actions is introduced:
CodeGenAction. We will be using this class to encapsulate logic shared
between all code-generation actions, but not required otherwise. For
now, it will:

  • run prescanning, parsing and semantic checks,
  • lower the input to MLIR.

EmitObjAction is updated to inherit from this class. This means that
the behaviour of flang-new -fc1 -emit-obj is also updated (previously,
it would just exit immediately). This change required
flang/test/Driver/syntax-only.f90 to be updated.

For -emit-fir, a specialisation of CodeGenAction is introduced:
EmitMLIRAction. The key logic for this class is implemented in
EmitMLIRAction::ExecuteAction.

Diff Detail

Event Timeline

awarzynski created this revision.Feb 4 2022, 3:26 AM
Herald added a project: Restricted Project. · View Herald Transcript
awarzynski requested review of this revision.Feb 4 2022, 3:26 AM

fir-dev PR: https://github.com/flang-compiler/f18-llvm-project/pull/1008. I've also incorporated a few small changes from the subsequent PRs.

awarzynski updated this revision to Diff 406005.Feb 4 2022, 9:30 AM

Add missing CMake dependency

schweitz accepted this revision.Feb 4 2022, 2:15 PM

LGTM

flang/include/flang/Frontend/FrontendActions.h
150

This appears in a header file. Should there be doxygen comments?

160

The LLVM coding conventions do not require trailing undescores.

This revision is now accepted and ready to land.Feb 4 2022, 2:15 PM
awarzynski added inline comments.Feb 7 2022, 2:55 AM
flang/include/flang/Frontend/FrontendActions.h
150

Good point, I'll add something here.

160

Good point, I'll change this.

awarzynski updated this revision to Diff 406377.Feb 7 2022, 3:19 AM

Removed trailing "_" from member variable names, added a comment, updated CMake dependencies (needed after https://reviews.llvm.org/D118966)

@schweitz, thank you for reviewing! If there are no new comments, I'll merge this today/tomorrow.

Few Nits/questions.

flang/lib/Frontend/FrontendActions.cpp
69

Nit: Can we remove the three autos below?

420

Nit: Should we test the existence of such a file?

421

Nit: can we remove auto?

flang/test/Driver/emit-mlir.f90
2

Nit: The the

flang/test/Driver/syntax-only.f90
16

Nit: Do you have another test for fsyntax-only?

23

Do we currently run the stages before codegen and then issue this error?

awarzynski added inline comments.Feb 7 2022, 4:28 AM
flang/lib/Frontend/FrontendActions.cpp
69

Sure!

420

We do :) Sort of!

To me, "existence" is a low level concept. Files are handled through e.g. llvm::raw_fd_ostream (and occasionally other streams) and IMO all low level details should be dealt with there (and indeed are). flang-new should only verify that os is not a nullptr. If the file does not exist, os will be a nullptr and that's checked further down. If the file does exist, then everything is fine and we can move to the next step.

421

Sure!

flang/test/Driver/emit-mlir.f90
2

Thanks :) Also, should be -emit-mlir instead of -emit-fir.

flang/test/Driver/syntax-only.f90
16
23

Yes, but this error is here only temporarily. I will be removing it shortly (once -emit-obj is implemented).

awarzynski updated this revision to Diff 406398.Feb 7 2022, 4:31 AM

Remove auto, update comment

kiranchandramohan accepted this revision.Feb 8 2022, 3:45 AM

LGTM.

flang/lib/Frontend/FrontendActions.cpp
420

OK, that was sounds fine. I was also meaning to ask whether we should have a test that a *.mlir file is generated in a test.

awarzynski added inline comments.Feb 8 2022, 7:42 AM
flang/lib/Frontend/FrontendActions.cpp
420

I can add one, yes!

awarzynski updated this revision to Diff 406828.Feb 8 2022, 7:48 AM
This revision was automatically updated to reflect the committed changes.