This is an archive of the discontinued LLVM Phabricator instance.

[flang][driver] Add support for -S and implement -c/-emit-obj
ClosedPublic

Authored by awarzynski on Feb 25 2022, 8:29 AM.

Details

Summary

This patch adds support for:

  • -S in Flang's compiler and frontend drivers

and implements (these options were already available as placeholders):

  • -emit-obj in Flang's frontend driver and -c in Flang's compiler driver

The semantics of these options in Clang and Flang are identical.

The EmitObjAction frontend action is renamed as BackendAction. The
new name more accurately reflects the fact that this action will
primarily run the code-gen/backend pipeline in LLVM. It also makes more
sense as an action implementing both -emit-obj and -S (originally it
was just -emit-obj).

tripleName in FirContext.cpp is updated from fir.triple to
llvm.target_triple. The former was effectively ignored. The latter is
used when lowering from the LLVM dialect in MLIR to LLVM IR (i.e. it's
embedded in the generated LLVM IR module). The driver can then re-use
that when configuring the backend. With this change, the LLVM IR files
generated by e.g. tco will from now on contain the correct target
triple.

The code-gen.f90 test is replaced with code-gen-x86.f90 and
code-gen-aarch64.f90. With 2 seperate files we can verify that
--target is correctly taken into account. LIT configuration is updated
to enable e.g.:

! REQUIRES: aarch64-registered-target

Diff Detail

Event Timeline

awarzynski created this revision.Feb 25 2022, 8:29 AM
awarzynski requested review of this revision.Feb 25 2022, 8:29 AM
schweitz added inline comments.Feb 25 2022, 9:00 AM
flang/lib/Frontend/FrontendActions.cpp
43

You'll want to keep in mind that some class names are overloaded between llvm and Fortran::xyz namespaces and even between Fortran::uvw and Fortran::xyz namespaces.

505
515

This assert comes after TM is dereferenced in the line above.

555

getTargetTriple() was already saved into theTriple variable above.

558

Do you want to assert on this RAII of TLII?

flang/lib/Optimizer/Support/FIRContext.cpp
19

Why is this being changed? This is a FIR specific attribute.

rovka added a comment.Feb 28 2022, 4:53 AM

I'm having trouble building this on Windows on Arm. I'm still investigating if it's a problem with the patch or just growing pains on the WoA side :) I'll try to get to the bottom of it as soon as I can.

flang/lib/Frontend/FrontendActions.cpp
508

Shouldn't this be a diagnostic? People could be trying to compile a random IR file with a triple that's not registered.

awarzynski marked 3 inline comments as done.Feb 28 2022, 6:22 AM
awarzynski added inline comments.
flang/lib/Frontend/FrontendActions.cpp
43

Thanks, I will revert this.

558

Any particular suggestion? I don't see anything that I could assert here.

flang/lib/Optimizer/Support/FIRContext.cpp
19

The driver assumes that when compiling a Fortran file, a valid triple will be present in the generated LLVM IR file. Currently, fir.tiple is just dropped and the generated LLVM IR module contains no triple. For -emit-obj/-S to work, we do need to make sure that a valid triple is actually there.

We could add logic to translate fir.triple to llvm.target_triple (in the FIR --> LLVM MLIR translation layer), but why bother if it's not used anywhere? Instead, Flang could use llvm.target_triple from the very beginning. This way, no extra functionality is needed (llvm.target_triple is preserved when translating from FIR to LLVM MLIR and then included in the generated LLVM IR module).

AFAIK, fir-triple is never used anywhere. In fact, it's already been removed from fir-dev.

Incorproate suggestions from Eric

Fix formatting and rebase

Update how target IR analysis passes are initialised (use TargetMachine::getTargetIRAnalysis() instead of calling the constructor directly).

rovka added a comment.Mar 1 2022, 12:59 AM

This passes on Windows on Arm if you add a forward declaration for class PassInstrumentation here. This is necessary because otherwise clang picks up the PassInstrumentation from MLIR instead of the PassInstrumentation defined lower in the file.

For reference, this is the error I was seeing before:

C:\Work\diana.picus\llvm-project\llvm\include\llvm/IR/PassInstrumentation.h(145,16): warning: unqualified friend declaration referring to type outside of the nearest enclosing namespace is a Microsoft extension; add a nested name specifier [-Wmicrosoft-unqualified-friend]
  friend class PassInstrumentation;
               ^
               ::mlir::
C:\Work\diana.picus\llvm-project\llvm\include\llvm/IR/PassInstrumentation.h(292,33): error: 'AnalysesClearedCallbacks' is a private member of 'llvm::PassInstrumentationCallbacks'
      for (auto &C : Callbacks->AnalysesClearedCallbacks)
                                ^
C:\Work\diana.picus\llvm-project\llvm\include\llvm/IR/PassInstrumentation.h(173,7): note: declared private here
      AnalysesClearedCallbacks;
awarzynski updated this revision to Diff 412008.Mar 1 2022, 1:31 AM

Add a forward declaration for class PassInstrumentation (thank you @rovka!)

Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2022, 1:31 AM
awarzynski added inline comments.Mar 1 2022, 2:34 AM
flang/lib/Frontend/FrontendActions.cpp
508

Good point! However, I'm struggling to trigger this assert ATM and I don't want to issue a diagnostic if I can't test it. This will change once flang-new can consume LLVM IR files (soon). In the meantime, let me add a TODO. Hopefully we won't forget to elevate this to a proper diagnostic when the time is right :)

rovka added a comment.Mar 1 2022, 3:59 AM

I don't have any other comments, I'll let Eric take it from here :)

flang/lib/Frontend/FrontendActions.cpp
508

TODO is fine for now, thanks.

schweitz added inline comments.Mar 1 2022, 1:58 PM
flang/lib/Frontend/FrontendActions.cpp
561

Got jumbled around somehow. TLII is allocated at line 559-560. Since you are checking allocations with assert in other places, why not here? TLII is immediately dereferenced on line 561, without an assertion check.

flang/lib/Optimizer/Support/FIRContext.cpp
19

Thanks for the update. I'm glad to see MLIR decided to use my module attribute trick here. :)

Given this, there is no reason to "block copy" the name of the attribute over to FIR. Just use the mlir::LLVM::LLVMDialect::getTargetTripleAttrName() method. The FIR attribute can then be retired and removed along with the setter and getter.

Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2022, 1:58 PM
awarzynski added inline comments.Mar 2 2022, 3:36 AM
flang/lib/Frontend/FrontendActions.cpp
561

Thanks for clarifying. One assert coming this way :)

flang/lib/Optimizer/Support/FIRContext.cpp
19

I wasn't aware of getTargetTripleAttrName, thanks! I will use that instead.

As for setTargetTriple and getTargetTriple, we'd need a bit more work to remove them. I suggest doing that in a separate patch.

awarzynski updated this revision to Diff 412386.Mar 2 2022, 5:50 AM
  • Got rid of tripleName
  • Added an assert for the newly created TLII
  • Rebased
  • Added #includes missing after the rebase
awarzynski updated this revision to Diff 412963.Mar 4 2022, 2:38 AM

Rebase on top of D120897

This revision was not accepted when it landed; it landed in state Needs Review.Mar 9 2022, 7:49 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
flang/test/Driver/code-gen-x86.f90