This is an archive of the discontinued LLVM Phabricator instance.

[Flang][Tool][bbc] Emit Module Wrapper in addition to body
ClosedPublic

Authored by agozillon on Feb 27 2023, 6:15 AM.

Details

Summary

This change seeks to emit the full module from the bbc tool and
not just the body. This change currently does not break existing tests
when running check-(mlir, flang, all).

For OpenMP offloading we are currently seeking to add attributes
directly to the builtin module and we are wondering why
the bbc tool discards the module, and if it is possible for the tool to
retain this information or if it would be a breaking change for other
uses of the tool?

These attributes tied to the module are likely required
to fully test the FIR lowering for OpenMP utilising bbc (I am
currently seeking to add an omp.is_device attribute to the
module and in a follow up an attribute holding RTL flags to
the module for later lowering). It's not an immediate concern
but I believe (without knowing the full picture at the moment)
it would be quite useful to retain all the information within the
FIR module, as whilst a builtin.module isn't part of FIR it has
and can have additional FIR/OpenMP related data appended
to it.

Diff Detail

Event Timeline

agozillon created this revision.Feb 27 2023, 6:15 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
agozillon requested review of this revision.Feb 27 2023, 6:15 AM

This will need a discussion with the FIR team.

agozillon edited the summary of this revision. (Show Details)Feb 27 2023, 6:20 AM
agozillon added a reviewer: kiranchandramohan.

This will need a discussion with the FIR team.

That's no problem at all and I'm sorry for the bother! Is there an ideal avenue for that e.g. adding it as a discussion point in one of the weekly meetings or is an RFC on discourse perhaps better?

adding it as a discussion point in one of the weekly meetings or is an RFC on discourse perhaps better?

We can briefly discuss it at the meeting tomorrow to check no one rely on not having the module printed.

The reason it is not printed are mostly historical at a time where the module contained nothing FIR relevant and the extra indentation caused by the module was annoying.
BTW, if there is an option to not add the extra indentation to the IR inside the module, I am in, since I mostly use bbc for visual inspection.

On my side I am ok with the change, it makes sense to reflect the compilation specific module attributes in the output.

adding it as a discussion point in one of the weekly meetings or is an RFC on discourse perhaps better?

We can briefly discuss it at the meeting tomorrow to check no one rely on not having the module printed.

That would be appreciated! Would this be the Flang bi-weekly sync-up or another meeting?

The reason it is not printed are mostly historical at a time where the module contained nothing FIR relevant and the extra indentation caused by the module was annoying.
BTW, if there is an option to not add the extra indentation to the IR inside the module, I am in, since I mostly use bbc for visual inspection.

From what I can tell there unfortunately isn't sadly, but my knowledge of LLVM/MLIR's pretty printing is a little lack luster.

On my side I am ok with the change, it makes sense to reflect the compilation specific module attributes in the output.

Thank you for your input!

From what I can tell there unfortunately isn't sadly, but my knowledge of LLVM/MLIR's pretty printing is a little lack luster.

Too bad, but not a blocker on my side. We will revisit pretty printing at some point to enable type alias usage for FIR derived types. Maybe we could see if it is possible to not add extra indentation to module content at that point.

That would be appreciated! Would this be the Flang bi-weekly sync-up or another meeting?

Yes, I think this can be brought up at today's sync to check no one relies on the current behavior.

jeanPerier accepted this revision.Mar 1 2023, 8:45 AM
This revision is now accepted and ready to land.Mar 1 2023, 8:45 AM

Thank you @jeanPerier for the help and review, that's it landed now, sorry for making the FIR IR reading harder for you for the time being!