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.

Sign up for GitHub

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

Jump to bottom

[flang][driver] Add support for -emit-obj in the frontend driver #1152

Merged
merged 1 commit into from Nov 1, 2021
Merged

[flang][driver] Add support for -emit-obj in the frontend driver #1152

merged 1 commit into from Nov 1, 2021

Conversation

banach-space
Copy link
Collaborator

Builds on top of #1113

@banach-space banach-space mentioned this pull request Oct 22, 2021
Merged
psteinfeld
psteinfeld reviewed Oct 22, 2021
View reviewed changes
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 can confirm that everything builds and passes tests. There are a few files (noted) that need to have clang-format run on them.

I'm not that familiar with the code so I'm not approving, but I didn't see anything wrong other than the clang-format problems.

flang/include/flang/Frontend/CompilerInstance.h Show resolved Hide resolved
flang/include/flang/Frontend/FrontendActions.h Outdated Show resolved Hide resolved
flang/include/flang/Tools/CLOptions.inc Outdated Show resolved Hide resolved
flang/lib/Frontend/FrontendActions.cpp Outdated Show resolved Hide resolved
clementval
clementval reviewed Oct 22, 2021
View reviewed changes
Copy link
Collaborator

@clementval clementval 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 working on this. Couple of small comments/questions.

The title mentions emit-obj but I see lots of emit-llvm so I'm a bit confused.

flang/include/flang/Tools/CLOptions.inc Outdated Show resolved Hide resolved
flang/include/flang/Tools/CLOptions.inc Outdated Show resolved Hide resolved
flang/lib/Frontend/CMakeLists.txt Outdated Show resolved Hide resolved
@banach-space
Copy link
Collaborator Author

@clementval, thanks for taking a look!

The title mentions emit-obj but I see lots of emit-llvm so I'm a bit confused.

-emit-llvm is implemented in #1113. This patch depends on #1113, so it includes the corresponding commit. I couldn't find a better way to express this dependency using GitHub's UI.

@clementval
Copy link
Collaborator

@clementval, thanks for taking a look!

The title mentions emit-obj but I see lots of emit-llvm so I'm a bit confused.

-emit-llvm is implemented in #1113. This patch depends on #1113, so it includes the corresponding commit. I couldn't find a better way to express this dependency using GitHub's UI.

Ok fine. That's right github does not have a depends on feature.

@jeanPerier
Copy link
Collaborator

Hi, I have just rebased fir-dev on LLVM 85bf221 from last week.
That means you will need to update this PR with the rebased fir-dev.
You cannot simply rebase your PR with fir-dev (it will try to reapply all fir-dev changes).

You can update the PR as follow (assuming your branch is called my-branch):

  • backup your branch into my-branch-bckup (just to be safe :) )
  • create a new branch my-branch-new following the new fir-dev head
  • cherry-pick all your commits from this PR into my-branch-new
  • once it looks good, force push my-branch-new into the upstream my-branch (git push origin my-branch-new:my-branch -f )

Sorry for the inconvenience, I could not wait for every PR to get in and solve all new conflicts on my side, the rebase
is meant to help upstreaming so that we can avoid having this too many times again in the future.

@banach-space
Copy link
Collaborator Author

banach-space commented Oct 28, 2021 •
edited

Rebased on top of the rebased fir-dev.

@banach-space
[flang][driver] Add support for -emit-obj in the frontend driver
4e6f6d7 
This patch implements the `EmitObjAction` frontend action. The
`CodeGenAction` is extended to contain an instance of `llvm::LLVMCtx`
and `llvm::Module`. These variables are unlikely to be needed in other,
non-code-gen actions (or in `CompilerInstance` or `CompilerInvocation`),
so these should be the right location. Also, a member method for
generating LLVM IR is added (see `GenerateLLVMIR`).

The target triple is hard-coded as `native`, i.e. no cross-compilation
is supported at this stage.

`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. As these tests are arch specific (they check the
generated assembly), support for `!REQUIRES: <arch>-registered-target`
was added in LIT. The current approach is a bit fragile and will cause
problems if multiple targets are available (e.g. X86 and AArch64).
That's because `!REQUIRES: X86-registered-target` will be satisfied even
on AArch64 targets if the X86 backend is enabled (through e.g.
`LLVM_ENABLE_TARGETS`). However, the test could fail as currently
`flang-new -fc1 -emit-obj` always generates code for the host
architecture. This can be fixed by adding support for `-target`.
@banach-space
Copy link
Collaborator Author

Rebased

@jeanPerier
Copy link
Collaborator

| Rebased on top of the rebased fir-dev.

Thanks, tested OK.

@jeanPerier jeanPerier merged commit 5e3c7fd into flang-compiler:fir-dev Nov 1, 2021
@banach-space banach-space deleted the andrzej/emit_obj branch November 1, 2021 11:43
@banach-space banach-space mentioned this pull request Nov 12, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@psteinfeld psteinfeld psteinfeld left review comments

@clementval clementval clementval approved these changes

@schweitzpgi schweitzpgi Awaiting requested review from schweitzpgi

@CarolineConcatto CarolineConcatto Awaiting requested review from CarolineConcatto

@jeanPerier jeanPerier Awaiting requested review from jeanPerier

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@banach-space @clementval @jeanPerier @psteinfeld