Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: FIR Merge - PR2: Add Fortran IR (FIR) MLIR dialect implementation #1035

Merged
merged 1 commit into from
Mar 12, 2020

Conversation

jeanPerier
Copy link
Collaborator

Add the FIR library that defines and implements the MLIR dialect used in flang. FIR is defined and documented inside FIROps.td in this commit.

The following documents can help to understand how MLIR dialects must be defined:

This patch adds the tool tco that processes FIR input files and drives transformations passes. The tco tool is used for testing. In this patch, the tco tool is to implement tests for parsing, printing, verifying, and round-tripping FIR.

Note:
This commit it a feature-based split of the work done in the FIR experimental branch. The related work log can be found in the commits between schweitzpgi@742edde and schweitzpgi@2ff5524.

RFC: http://lists.llvm.org/pipermail/flang-dev/2020-January/000162.html

Copy link

@River707 River707 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Jean, I hope you don't mind but I left some comments. I still haven't been able to look through all of it though.

include/flang/Optimizer/Dialect/FIROps.td Outdated Show resolved Hide resolved
include/flang/Optimizer/Dialect/FIROps.td Outdated Show resolved Hide resolved
include/flang/Optimizer/Dialect/FIROps.td Outdated Show resolved Hide resolved
include/flang/Optimizer/Dialect/FIROps.td Outdated Show resolved Hide resolved
include/flang/Optimizer/Dialect/FIROps.td Show resolved Hide resolved
include/flang/Optimizer/Dialect/FIROps.td Outdated Show resolved Hide resolved
include/flang/Optimizer/Dialect/FIRType.h Outdated Show resolved Hide resolved
lib/Optimizer/Dialect/FIRAttr.cpp Show resolved Hide resolved
lib/Optimizer/Dialect/FIRAttr.cpp Outdated Show resolved Hide resolved
tools/tco/tco.cpp Outdated Show resolved Hide resolved
@schweitzpgi
Copy link
Collaborator

Hi Jean, I hope you don't mind but I left some comments. I still haven't been able to look through all of it though.

Thanks for the helpful tips, River. Much appreciated.

@tskeith
Copy link
Collaborator

tskeith commented Feb 28, 2020

Can you update the build instructions? My LLVM build doesn't have AddMLIR.cmake but the install does. But the install doesn't have llvm-lit.

@tskeith
Copy link
Collaborator

tskeith commented Feb 28, 2020

The tests fail for me with: tco: command not found

test-lit/Fir/fir-ops.fir Show resolved Hide resolved
include/flang/Optimizer/Support/KindMapping.h Outdated Show resolved Hide resolved
include/flang/Optimizer/Support/KindMapping.h Outdated Show resolved Hide resolved
include/flang/Optimizer/Dialect/FIRType.h Show resolved Hide resolved
lib/Optimizer/Dialect/FIRType.cpp Outdated Show resolved Hide resolved
lib/Optimizer/Dialect/FIROps.cpp Outdated Show resolved Hide resolved
lib/Optimizer/Dialect/FIROps.cpp Outdated Show resolved Hide resolved
lib/Optimizer/Dialect/FIROps.cpp Outdated Show resolved Hide resolved
lib/Optimizer/Dialect/FIROps.cpp Outdated Show resolved Hide resolved
lib/Optimizer/Dialect/FIRAttr.cpp Outdated Show resolved Hide resolved
@jeanPerier
Copy link
Collaborator Author

Can you update the build instructions? My LLVM build doesn't have AddMLIR.cmake but the install does. But the install doesn't have llvm-lit.

The tests fail for me with: tco: command not found

Thanks for testing, I have updated the README and also changed the cmake files so that tco is always built when LINK_WITH_FIR=On is set (which is default).

Copy link
Collaborator

@DavidTruby DavidTruby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This patch is still quite large and so difficult to review thoroughly. Is there any way to divide it further?

include/fir/.clang-format Show resolved Hide resolved
include/flang/Optimizer/Dialect/FIRAttr.h Outdated Show resolved Hide resolved
include/flang/Optimizer/Dialect/FIROps.h Outdated Show resolved Hide resolved
include/flang/Optimizer/Dialect/FIROps.h Outdated Show resolved Hide resolved
include/flang/Optimizer/Dialect/FIROps.td Show resolved Hide resolved
lib/Optimizer/CMakeLists.txt Show resolved Hide resolved
lib/Optimizer/Dialect/FIROps.cpp Show resolved Hide resolved
lib/Optimizer/Dialect/FIROps.cpp Show resolved Hide resolved
lib/Optimizer/Dialect/FIRType.cpp Show resolved Hide resolved
Copy link
Collaborator

@kiranchandramohan kiranchandramohan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR. I have a few comments/questions.

Why is the LangRef not part of this? If this is autogen then can you add a pointer to how to access that?
Is there no representation for co-arrays in FIR now?

lib/Optimizer/Dialect/FIRType.cpp Outdated Show resolved Hide resolved
include/flang/Optimizer/Dialect/FIROps.h Outdated Show resolved Hide resolved
include/flang/Optimizer/Dialect/FIROps.h Outdated Show resolved Hide resolved
include/flang/Optimizer/Dialect/FIRType.h Outdated Show resolved Hide resolved
lib/Optimizer/Dialect/FIRDialect.cpp Outdated Show resolved Hide resolved
include/flang/Optimizer/Dialect/FIROps.td Show resolved Hide resolved
include/flang/Optimizer/Dialect/FIROps.td Show resolved Hide resolved
include/flang/Optimizer/Dialect/FIROps.td Show resolved Hide resolved
include/flang/Optimizer/Dialect/FIROps.td Show resolved Hide resolved
lib/Optimizer/Dialect/FIRType.cpp Outdated Show resolved Hide resolved
@jeanPerier
Copy link
Collaborator Author

This patch is still quite large and so difficult to review thoroughly. Is there any way to divide it further?

While this patch is bigger than usual f18 patches, it is centered around only one feature: FIR. We already removed all the FIR transformations passes, the lowering to LLVM dialect and also the lowering to FIR. We think that removing more would make it harder to understand what is FIR. It is also not possible to remove implementation parts while keeping this patch testable.

If you cannot make a thorough review, I would advise focusing on FIROps.td, and FIRType.h that are the central pieces of this PR, the rest is mainly here to demonstrate it live.

README.md Outdated Show resolved Hide resolved
@jeanPerier
Copy link
Collaborator Author

jeanPerier commented Mar 6, 2020

Why is the LangRef not part of this? If this is autogen then can you add a pointer to how to access that?

Good point, it was not part of this PR because it require add_mlir_function to generate it but as mentioned in an earlier comment of @DavidTruby we cannot use it (it seems to only works for dialect whose sources live inside MLIR sources). So, I have replicated the part to generate the FIRLangRef.md and updated the README.md

@jeanPerier
Copy link
Collaborator Author

Is there no representation for co-arrays in FIR now?

That's correct. We want to gain more experience with both FIR and co-arrays before deciding how they should be handled in FIR.

@jeanPerier
Copy link
Collaborator Author

Thanks all for your reviews !
We should have addressed all your comments so far (either solved, rejected, or agreed to solve them in later PRs as improvement). Please do not hesitate replying to this if you feel some of your comments were forgotten. Otherwise you are welcome to approve this PR so we can move to the next patches !

Copy link
Collaborator

@psteinfeld psteinfeld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't looked at the code itself, but after building LLVM, everything builds and tests correctly on my machine using GNU 8.3.0.

@kiranchandramohan kiranchandramohan self-requested a review March 9, 2020 12:35
Copy link
Collaborator

@kiranchandramohan kiranchandramohan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing the review comments. LGTM.

It will great if you can,

  1. Update the commit message with details of how to generate the documentation.
  2. Spend some time to change or replace the original FIR investigation documentation
    https://github.com/flang-compiler/f18/blob/master/documentation/Investigating-FIR-as-an-MLIR-dialect.md
  3. Convey in the commit message what exactly is there in this patch and what is not. For e.g.,
    -> contains the definition, printing, parsing and verification of operations.
    -> is the verification of all operations there in this patch?
    -> Co-arrays are not there.

@jeanPerier
Copy link
Collaborator Author

Thanks for addressing the review comments. LGTM.

It will great if you can,

  1. Update the commit message with details of how to generate the documentation.
  2. Spend some time to change or replace the original FIR investigation documentation
    https://github.com/flang-compiler/f18/blob/master/documentation/Investigating-FIR-as-an-MLIR-dialect.md
  3. Convey in the commit message what exactly is there in this patch and what is not. For e.g.,
    -> contains the definition, printing, parsing and verification of operations.
    -> is the verification of all operations there in this patch?
    -> Co-arrays are not there.

Thanks @kiranchandramohan , I have removed https://github.com/flang-compiler/f18/blob/master/documentation/Investigating-FIR-as-an-MLIR-dialect.md because it was an early-design document that had a purpose when we started looking at FIR but is now outdated, thanks for pointing to it. The info inside is replaced by FIRLangRef.md that can be generated and new documents that will come with the next patches.

When squashing and rebasing for the merge, I will change the commit to:

Add Fortran IR (FIR) MLIR dialect implementation

Adds FIR library that implements an MLIR dialect to which Fortran
parse-tree will be lowered to.

FIR is defined and documented inside FIROps.td added in this commit.
It is possible to generate a more readable description FIRLangRef.md
from FIROps.td following the related instructions added to the README.md
by this commit.

This patch adds FIR definition and implementation that allow parsing,
printing, and verifying FIR. FIR transformations and lowering to Standard
and LLVM dialects are not part of this patch. The FIR verifiers are verifying
the basic properties of FIR operations in order to provide a sufficient
frame for lowering. Verifiers for more advanced FIR properties can be added
as needed.

Coarrays are not covered by FIR defined in this patch.

This patch also adds tco tool that is meant to process FIR input files and
drives transformations on it. The tco tool is used for testing.
In this patch, it is only used to demonstrate parsing/verifying/
and dumping FIR with round-trip tests.



Note:
This commit does not reflect an actual work log, it is a feature-based split of the
changes done in the FIR experimental branch. The related work log can be found in the
commits between:

https://github.com/schweitzpgi/f18/commit/742edde572bd74d77cf7d447132ccf0949187fce
and
https://github.com/schweitzpgi/f18/commit/2ff55242126d86061f4fed9ef7b59d3636b5fd0b

Changes on top of these original commits were made during this patch review.

@jeanPerier
Copy link
Collaborator Author

jeanPerier commented Mar 11, 2020

I have pushed an update to adapt with the latest changes in MLIR (since end-of last week, this PR was could not be build with LLVM head due to MLIR interface changes). The f18 branch of f18-llvm-project LLVM fork mentioned in the README was updated accordingly, but the current head from llvm-project also works with FIR at the time of this comment.

The other commit I pushed partially addresses @River707 comment that suggested moving big code pieces from the .td to a .cpp file (the comment was deferred, but the need to rebase with latest LLVM head gave us time to to it). [Edit: not all big pieces are moved yet, they will be in parallel of related updates]

Adds FIR library that implements an MLIR dialect to which Fortran
parse-tree will be lowered to.

FIR is defined and documented inside FIROps.td added in this commit.
It is possible to generate a more readable description FIRLangRef.md
from FIROps.td following the related instructions added to the README.md
by this commit.

This patch adds FIR definition and implementation that allow parsing,
printing, and verifying FIR. FIR transformations and lowering to Standard
and LLVM dialects are not part of this patch. The FIR verifiers are verifying
the basic properties of FIR operations in order to provide a sufficient
frame for lowering. Verifiers for more advanced FIR properties can be added
as needed.

Coarrays are not covered by FIR defined in this patch.

This patch also adds tco tool that is meant to process FIR input files and
drives transformations on it. The tco tool is used for testing.
In this patch, it is only used to demonstrate parsing/verifying/
and dumping FIR with round-trip tests.

Note:
This commit does not reflect an actual work log, it is a feature-based split of the
changes done in the FIR experimental branch. The related work log can be found in the
commits between:

schweitzpgi@742edde
and
schweitzpgi@2ff5524

Changes on top of these original commits were made during this patch review.
@jeanPerier
Copy link
Collaborator Author

I have squashed and rebased to prepare merging, if you still need the commit history for the comments, you can find it here: https://github.com/flang-compiler/f18/commits/d0a7dabd6941a6725436e1c37ccc6e2b9aa0d6fb

Copy link
Member

@sscalpone sscalpone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Builds and tests clean for me against llvm head of master.

@sscalpone sscalpone merged commit 30b428a into master Mar 12, 2020
@sscalpone sscalpone deleted the adding-fir branch March 12, 2020 04:47
@psteinfeld
Copy link
Collaborator

Similar to @DavidTruby, after pulling in these change, I cannot build. I was able to build successfully a couple of days ago before these changes were merged into master. My build environment is using GNU 8.3.0 and my own built version of LLVM, from the latest sources as of about a week ago, which is version 11.0.0.

The build fails very quickly. Here's a fragment of the log file, starting at the beginning:

[1/190] Building FIROps.h.inc...
FAILED: include/flang/Optimizer/Dialect/FIROps.h.inc 
cd /mnt/c/GitHub/f18/master/Release && /mnt/c/GitHub/f18/fir/install/bin/mlir-tblgen -gen-op-decls -I -I -I /mnt/c/GitHub/f18/master/include/flang/Optimizer/Dialect -I /mnt/c/GitHub/f18/fir/install/include /mnt/c/GitHub/f18/master/include/flang/Optimizer/Dialect/FIROps.td --write-if-changed -o include/flang/Optimizer/Dialect/FIROps.h.inc -d include/flang/Optimizer/Dialect/FIROps.h.inc.d
/mnt/c/GitHub/f18/master/include/flang/Optimizer/Dialect/FIROps.td:17:9: error: Could not find include file 'mlir/Interfaces/ControlFlowInterfaces.td'
include "mlir/Interfaces/ControlFlowInterfaces.td"
        ^
/mnt/c/GitHub/f18/master/include/flang/Optimizer/Dialect/FIROps.td:17:9: error: Unexpected input at top level
include "mlir/Interfaces/ControlFlowInterfaces.td"
        ^
[2/190] Building FIROps.cpp.inc...
FAILED: include/flang/Optimizer/Dialect/FIROps.cpp.inc 
cd /mnt/c/GitHub/f18/master/Release && /mnt/c/GitHub/f18/fir/install/bin/mlir-tblgen -gen-op-defs -I -I -I /mnt/c/GitHub/f18/master/include/flang/Optimizer/Dialect -I /mnt/c/GitHub/f18/fir/install/include /mnt/c/GitHub/f18/master/include/flang/Optimizer/Dialect/FIROps.td --write-if-changed -o include/flang/Optimizer/Dialect/FIROps.cpp.inc -d include/flang/Optimizer/Dialect/FIROps.cpp.inc.d
/mnt/c/GitHub/f18/master/include/flang/Optimizer/Dialect/FIROps.td:17:9: error: Could not find include file 'mlir/Interfaces/ControlFlowInterfaces.td'
include "mlir/Interfaces/ControlFlowInterfaces.td"
        ^
/mnt/c/GitHub/f18/master/include/flang/Optimizer/Dialect/FIROps.td:17:9: error: Unexpected input at top level
include "mlir/Interfaces/ControlFlowInterfaces.td"
        ^
[3/190] Building CXX object lib/Common/CMakeFiles/FortranCommon.dir/Fortran.cpp.o
   ...

@DavidTruby
Copy link
Collaborator

@psteinfeld, @jeanPerier mentioned to me that MLIR's interface changed on Tuesday. So you need a build of LLVM from yesterday or today for this to work correctly. Once I built LLVM from scratch from a new git clone, it worked fine.

@psteinfeld
Copy link
Collaborator

@psteinfeld, @jeanPerier mentioned to me that MLIR's interface changed on Tuesday. So you need a build of LLVM from yesterday or today for this to work correctly. Once I built LLVM from scratch from a new git clone, it worked fine.

Thanks! I'll give it a try.

@psteinfeld
Copy link
Collaborator

@psteinfeld, @jeanPerier mentioned to me that MLIR's interface changed on Tuesday. So you need a build of LLVM from yesterday or today for this to work correctly. Once I built LLVM from scratch from a new git clone, it worked fine.

Thanks! I'll give it a try.

I cleaned out my local LLVM build, rebuilt the latest from scratch, and now everything works. Thanks for your help!

Note that my first attempt was to simple grab the latest sources from GitHub, run cmake, and rebuild. That wasn't good enough. I had to delete the build area and do everything from scratch.

How frequently should I expect to need to rebuild LLVM?

@DavidTruby
Copy link
Collaborator

Once the new cmake changes land and we are part of the llvm monorepo, the easiest way of managing things should be to build alongside llvm and we shouldn't have these issues. See #1045 for more info!

@psteinfeld
Copy link
Collaborator

Once the new cmake changes land and we are part of the llvm monorepo, the easiest way of managing things should be to build alongside llvm and we shouldn't have these issues. See #1045 for more info!

Thanks, David! You're up past my bed time.

Sameeranjoshi pushed a commit to Sameeranjoshi/f18 that referenced this pull request Mar 24, 2020
Adds FIR library that implements an MLIR dialect to which Fortran
parse-tree will be lowered to.

FIR is defined and documented inside FIROps.td added in this commit.
It is possible to generate a more readable description FIRLangRef.md
from FIROps.td following the related instructions added to the README.md
by this commit.

This patch adds FIR definition and implementation that allow parsing,
printing, and verifying FIR. FIR transformations and lowering to Standard
and LLVM dialects are not part of this patch. The FIR verifiers are verifying
the basic properties of FIR operations in order to provide a sufficient
frame for lowering. Verifiers for more advanced FIR properties can be added
as needed.

Coarrays are not covered by FIR defined in this patch.

This patch also adds tco tool that is meant to process FIR input files and
drives transformations on it. The tco tool is used for testing.
In this patch, it is only used to demonstrate parsing/verifying/
and dumping FIR with round-trip tests.

Note:
This commit does not reflect an actual work log, it is a feature-based split of the
changes done in the FIR experimental branch. The related work log can be found in the
commits between:

schweitzpgi@742edde
and
schweitzpgi@2ff5524

Changes on top of these original commits were made during this patch review.
swift-ci pushed a commit to swiftlang/llvm-project that referenced this pull request Apr 9, 2020
…ler/f18#1035)

Adds FIR library that implements an MLIR dialect to which Fortran
parse-tree will be lowered to.

FIR is defined and documented inside FIROps.td added in this commit.
It is possible to generate a more readable description FIRLangRef.md
from FIROps.td following the related instructions added to the README.md
by this commit.

This patch adds FIR definition and implementation that allow parsing,
printing, and verifying FIR. FIR transformations and lowering to Standard
and LLVM dialects are not part of this patch. The FIR verifiers are verifying
the basic properties of FIR operations in order to provide a sufficient
frame for lowering. Verifiers for more advanced FIR properties can be added
as needed.

Coarrays are not covered by FIR defined in this patch.

This patch also adds tco tool that is meant to process FIR input files and
drives transformations on it. The tco tool is used for testing.
In this patch, it is only used to demonstrate parsing/verifying/
and dumping FIR with round-trip tests.

Note:
This commit does not reflect an actual work log, it is a feature-based split of the
changes done in the FIR experimental branch. The related work log can be found in the
commits between:

schweitzpgi/obsolete-f18@742edde
and
schweitzpgi/obsolete-f18@2ff5524

Changes on top of these original commits were made during this patch review.

Original-commit: flang-compiler/f18@30b428a
Reviewed-on: flang-compiler/f18#1035
mem-frob pushed a commit to draperlaboratory/hope-llvm-project that referenced this pull request Oct 7, 2022
…ler/f18#1035)

Adds FIR library that implements an MLIR dialect to which Fortran
parse-tree will be lowered to.

FIR is defined and documented inside FIROps.td added in this commit.
It is possible to generate a more readable description FIRLangRef.md
from FIROps.td following the related instructions added to the README.md
by this commit.

This patch adds FIR definition and implementation that allow parsing,
printing, and verifying FIR. FIR transformations and lowering to Standard
and LLVM dialects are not part of this patch. The FIR verifiers are verifying
the basic properties of FIR operations in order to provide a sufficient
frame for lowering. Verifiers for more advanced FIR properties can be added
as needed.

Coarrays are not covered by FIR defined in this patch.

This patch also adds tco tool that is meant to process FIR input files and
drives transformations on it. The tco tool is used for testing.
In this patch, it is only used to demonstrate parsing/verifying/
and dumping FIR with round-trip tests.

Note:
This commit does not reflect an actual work log, it is a feature-based split of the
changes done in the FIR experimental branch. The related work log can be found in the
commits between:

schweitzpgi/obsolete-f18@742edde
and
schweitzpgi/obsolete-f18@2ff5524

Changes on top of these original commits were made during this patch review.

Original-commit: flang-compiler/f18@30b428a
Reviewed-on: flang-compiler/f18#1035
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants