Page MenuHomePhabricator

[flang][driver] Make `flang-new -fc1` accept MLIR files
ClosedPublic

Authored by awarzynski on Jun 2 2022, 9:18 AM.

Details

Summary

This relatively small change will allow Flang's frontend driver,
flang-new -fc1, to consume and parse MLIR files. Semantically (i.e.
from user's perspective) this is identical to reading LLVM IR files.

Two file extensions are associated with MLIR files: .fir and .mlir. Note
that reading MLIR files makes only sense when running one of the
code-generation actions, i.e. when using one of the following action
flags: -S, -emit-obj, -emit-llvm, -emit-llvm-bc.

The majority of tests that required tco to run are updated to also run
with flang-new -fc1. A few tests are updated to use fir-opt instead
of tco (that's the preferred choice when testing a particular MLIR
pass). basic-program.fir is not updated as that test is intended to
verify the behaviour of tco specifically.

Diff Detail

Event Timeline

awarzynski created this revision.Jun 2 2022, 9:18 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
awarzynski requested review of this revision.Jun 2 2022, 9:18 AM
rovka added a comment.Jun 3 2022, 2:52 AM

This looks good in general, but it's missing a couple of tests:

  • a test with a file with a .mlir extension
  • tests for the -x option

Also you could commit some of the unrelated test cleanups and tco -> fir-opt or tco -> flang transitions separately, right?

flang/include/flang/Frontend/FrontendOptions.h
128

Nit: Does this need a space before the '@'?

flang/lib/Frontend/CompilerInvocation.cpp
277

What's the difference between this switch and the one right above at line 265?

Thanks for taking a look!

This looks good in general, but it's missing a couple of tests:

  • a test with a file with a .mlir extension

Sure, can do! Shall I add one with a .fir extension too?

  • tests for the -x option

Well, the frontend driver doesn't really understand -x just yet:

bin/flang-new -fc1 -S -x ir file.dx -o -
error: unknown argument: '-x'

We'd need to add FC1Option to the definition of -x. But that should be a separate patch 🤔 .

Also you could commit some of the unrelated test cleanups and tco -> fir-opt or tco -> flang transitions separately, right?

Sure, see https://reviews.llvm.org/D126955.

flang/lib/Frontend/CompilerInvocation.cpp
277

No difference :) It's there the split the logic a bit.

This doesn't make that much sense in Flang (because it only supports a handful of input types), but this code was most likely inspired by Clang, in which case the separation helps to organise the code.

awarzynski updated this revision to Diff 433999.Jun 3 2022, 4:53 AM

Address review comments

peixin added a comment.Jun 4 2022, 4:52 AM

It seems that generating executable file from MLIR file is not supported in this patch, right? Will it be supported in future?

awarzynski updated this revision to Diff 434792.Jun 7 2022, 5:42 AM

Add a test for -x

Note that this requires making this revision depend on
https://reviews.llvm.org/D127207

It seems that generating executable file from MLIR file is not supported in this patch, right?

As in, flang-new file.mlir --> executable? No. But you can lower your MLIR/FIR files to object files and just link them.

Will it be supported in future?

I don't see why not. We just need somebody to implement it ;-)

Is it the right trade-off between test coverage and computational effort to have dual run lines for the tco cases? I would assume that in most cases, what is actually tested will be deferred to the same code by both tools. Could we get almost the same coverage for half the effort by using the one tool for half the tests and the other for the rest, or would we miss something important by doing that?

(ping @clementval, as you expressed opinions on a similar matter in D126955)

peixin added a comment.Jun 7 2022, 6:09 PM

It seems that generating executable file from MLIR file is not supported in this patch, right?

As in, flang-new file.mlir --> executable? No. But you can lower your MLIR/FIR files to object files and just link them.

Will it be supported in future?

I don't see why not. We just need somebody to implement it ;-)

OK. Got it. Thanks.

Is it the right trade-off between test coverage and computational effort to have dual run lines for the tco cases?

Running tests is fast :) Also, there's not that many tests in Flang and this patch only affects a small fraction of them. And given Flang build times, I think that it is safe to assume that the cost is insignificant.

I would assume that in most cases, what is actually tested will be deferred to the same code by both tools. Could we get almost the same coverage for half the effort by using the one tool for half the tests and the other for the rest, or would we miss something important by doing that?

Both tools drive the same set of libraries, but these libraries are very complex (i.e. require a lot of set-up that depends on user input) and the drivers are almost completely separate. It is hard to guarantee that the behavior is consistent without testing with both (in fact, we did see them diverge in the past - that happens when updates are made in one tool, but not in the other).

rovka accepted this revision.Jun 8 2022, 3:43 AM

This LGTM, but please wait another day or two before committing in case anyone else still has comments.

flang/lib/Frontend/CompilerInvocation.cpp
277

Ok, but then the comment about suffixes is very obscure. Can we replace it with something clearer?

This revision is now accepted and ready to land.Jun 8 2022, 3:43 AM
awarzynski updated this revision to Diff 435116.Jun 8 2022, 5:50 AM

Update comment in parseFrontendArgs

ekieri added a comment.Jun 8 2022, 9:16 AM

Both tools drive the same set of libraries, but these libraries are very complex (i.e. require a lot of set-up that depends on user input) and the drivers are almost completely separate. It is hard to guarantee that the behavior is consistent without testing with both (in fact, we did see them diverge in the past - that happens when updates are made in one tool, but not in the other).

Thanks for explaining! I agree testing both is important in this situation.

Although running a test is fast, running all of them takes a while, and flang's test suite will grow over time. As long as the tests add coverage this is as it should be, just wanted to make sure this was the case here.

Both tools drive the same set of libraries, but these libraries are very complex (i.e. require a lot of set-up that depends on user input) and the drivers are almost completely separate. It is hard to guarantee that the behavior is consistent without testing with both (in fact, we did see them diverge in the past - that happens when updates are made in one tool, but not in the other).

Thanks for explaining! I agree testing both is important in this situation.

Thanks, can I assume that you are OK with this change?

Although running a test is fast, running all of them takes a while, and flang's test suite will grow over time.

Running tests is fast because it's something that's is trivial to parallelize. And one needs many beefy cores to build Flang anyway (sadly) :/ Also, there's ~1.5k tests in Flang ATM. ~16k in Clang. Not counting MLIR and LLVM. So this shouldn't really be a problem for a while.

Thanks, can I assume that you are OK with this change?

Yes! I have not had the time to read it closely enough for the four magic letters, but no objections from my side.

Although running a test is fast, running all of them takes a while, and flang's test suite will grow over time.

Running tests is fast because it's something that's is trivial to parallelize. And one needs many beefy cores to build Flang anyway (sadly) :/ Also, there's ~1.5k tests in Flang ATM. ~16k in Clang. Not counting MLIR and LLVM. So this shouldn't really be a problem for a while.

Haha, my home computer is very much inappropriate for this.

Haha, my home computer is very much inappropriate for this.

Your patience for building LLVM Flang is greatly appreciated! :D

By the way, did you note that the CI has some trouble with the formatting?

By the way, did you note that the CI has some trouble with the formatting?

Yes, but haven't noticed anything in the artifacts :(

This revision was landed with ongoing or failed builds.Jun 10 2022, 3:59 AM
This revision was automatically updated to reflect the committed changes.