-
Notifications
You must be signed in to change notification settings - Fork 48
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
Conversation
There was a problem hiding this 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.
Thanks for the helpful tips, River. Much appreciated. |
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: |
Thanks for testing, I have updated the README and also changed the cmake files so that |
There was a problem hiding this 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?
There was a problem hiding this 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?
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 |
Good point, it was not part of this PR because it require |
That's correct. We want to gain more experience with both FIR and co-arrays before deciding how they should be handled in FIR. |
Thanks all for your reviews ! |
There was a problem hiding this 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.
There was a problem hiding this 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,
- Update the commit message with details of how to generate the documentation.
- 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 - 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:
|
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 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.
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 |
There was a problem hiding this 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.
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:
|
@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? |
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. |
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.
…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
…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
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:
FIROps.td
,FIROpsSupport.h
, andFIROps.h
.FIRAttr.h
andFIRType.h
.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