This is an archive of the discontinued LLVM Phabricator instance.

[flang][driver] Add support for the `-emit-llvm` option
ClosedPublic

Authored by awarzynski on Feb 4 2022, 9:25 AM.

Details

Summary

This patch adds support for the -emit-llvm option in the frontend
driver (i.e. flang-new -fc1). Similarly to Clang, `flang-new -fc1
-emit-llvm file.f` will generate a textual LLVM IR file.

Depends on D118985

Diff Detail

Event Timeline

awarzynski created this revision.Feb 4 2022, 9:25 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: dang. · View Herald Transcript
awarzynski requested review of this revision.Feb 4 2022, 9:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 4 2022, 9:25 AM
awarzynski updated this revision to Diff 406380.Feb 7 2022, 3:30 AM

Rebase, remove trailing "_" from member variable names

rovka added inline comments.Feb 7 2022, 3:44 AM
flang/include/flang/Frontend/FrontendOptions.h
37

Shouldn't this be .ll?

flang/lib/Frontend/FrontendActions.cpp
422

Nit: Should we assert that mlirModule exists?
Also, why doesn't it already have a triple and a kind mapping?

465

This looks like an obsolete comment.

flang/test/Driver/emit-llvm.f90
2
awarzynski added inline comments.Feb 7 2022, 6:11 AM
flang/lib/Frontend/FrontendActions.cpp
422

Nit: Should we assert that mlirModule exists?

We could do, yes. However, mlirModule is created in CodeGenAction::BeginSourceFileAction. If that method fails, the driver should stop immediately. So perhaps that would be a better for place for an assert?

On a related note, mlirModule is obtained via LoweringBridge::getModule. But if the corresponding module is nullptr than that getter should probably assert. See https://reviews.llvm.org/D119133.

Also, why doesn't it already have a triple and a kind mapping?

Good catch, this is not needed yet. I didn't notice this when going over tco.

awarzynski updated this revision to Diff 406425.Feb 7 2022, 6:19 AM
awarzynski marked 2 inline comments as done.

Update comments + add an assert

awarzynski updated this revision to Diff 406428.Feb 7 2022, 6:25 AM

Remove the calls to setTargetTriple and setKindMapping.

rovka accepted this revision.Feb 8 2022, 12:22 AM

LGTM, thanks! I just have a minor nit, feel free to ignore it.

flang/lib/Frontend/FrontendActions.cpp
472

Nit: Can this be folded into the above if? (I.e. just write to os if you managed to create it, and add an else branch for the other case)

This revision is now accepted and ready to land.Feb 8 2022, 12:22 AM

The tests added are failing in the windows CI.

flang/lib/Frontend/FrontendActions.cpp
422

D118985 creates a bridge with an empty triple. The patch here was switching it to "native". Please cross-check what the expected behaviour.

lower::LoweringBridge lb = Fortran::lower::LoweringBridge::create(*mlirCtx,
    defKinds, ci.invocation().semanticsContext().intrinsics(),
    ci.parsing().allCooked(), "", kindMap);
441

Nit: Remove auto.

flang/unittests/Frontend/FrontendActionTest.cpp
176

Nit: Is the size important?

I believe that Windows failures are due to the missing support here: https://github.com/llvm/llvm-project/blob/81cde474e2c5a6280cb693b777ddcf473825ae8a/flang/lib/Optimizer/CodeGen/Target.cpp#L290. I can disable the LIT test on Windows, but I'm not sure how to do it for unit tests.

flang/lib/Frontend/FrontendActions.cpp
422

Please cross-check what the expected behaviour.

That's a good point, Kiran! Thanks :)

The constructor for LoweringBridge will call fir::setTargetTriple. This, in turn, will call fir::determineTargetTriple, which (for an empty triple) selects the target as follows:

if (triple.empty() || triple == "default")
  return llvm::sys::getDefaultTargetTriple()

AFAIK, native and default are compatible (I couldn't find any indication otherwise). So, empty triple above and "native" will lead to the same thing. So we are still just using native here.

This made me realise though that we should be explicit in D118985 and set the triple there. I'll update it shortly.

441

Sure! I will also rename this as moduleName.

472

Good suggestion, thanks!

I recall having a good reason to write it like this, but don't remember what it was. Let me simplify!

flang/unittests/Frontend/FrontendActionTest.cpp
176

No, thanks for noting this!

In fact, the generated output is larger than 256 characters.

awarzynski updated this revision to Diff 406832.Feb 8 2022, 7:58 AM

Disable the LIT test on Windows, simplify how output is dumped in EmitLLVMAction::ExecuteAction, remove auto

Rebase on top of main

awarzynski updated this revision to Diff 407130.Feb 9 2022, 5:41 AM

Updated fir::CodeGenSpecifics::get to see whether the unit tests pass on Windows. If they do, I'll create a seperate patch with this change.

awarzynski updated this revision to Diff 407139.Feb 9 2022, 6:25 AM

Remove the change from fir::CodeGenSpecifics::get (this was uploaded as a
seperate patch: https://reviews.llvm.org/D119332)

A believe that all of Kiran's comments have been addressed and pre-merge CI is also passing now (that was one of the concerns). Kiran has not accepted this yet, but he will be away for a few weeks. I will merge this as is to unblock further work. I more than happy to address any post-commit comments, Kiran!

This revision was automatically updated to reflect the committed changes.