Page MenuHomePhabricator

[flang] add -flang-experimental-hlfir flag to flang-new

Authored by tblah on Fri, Mar 17, 2:37 AM.



This flag instructs flang-new to use the new HLFIR lowering. It is
marked as experimental and not included in --help.

This was added to make it more convenient to test the performance of
code generated by the HLFIR lowering.

Diff Detail

Event Timeline

tblah created this revision.Fri, Mar 17, 2:37 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
Herald added a subscriber: sunshaoce. · View Herald Transcript
tblah requested review of this revision.Fri, Mar 17, 2:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Mar 17, 2:37 AM

Could you add some tests?

Thank you for working on this!


Would you mind also calling this in bbc driver?


Is this check and the option really needed? Except for the extra canonicalizer pass the newly added passes will be no-ops, so maybe we should just run them unconditionally.

It may be too much to ask for, but will it make sense not to bundle these passes into MLIRToLLVM pipeline and have the possibility to let driver emit post-HLFIR-lowering MLIR (under -emit-fir) and pre-HLFIR-lowering (under some new option)?

vzakhari added inline comments.Fri, Mar 17, 5:18 PM

I would imagine we may not want to optimize MATMUL(TRANSPOSE) into MATMUL_TRANSPOSE at O0. What is the best way to control this? We may either disable canonicalization or let LowerHLFIRIntrinsicsPass lower MATMUL_TRANSPOSE differently based on the optimization level. Or is it always okay to implement it as a combined operation?

You can either pass parameters to passes or use different pass pipelines for different optimisation levels (in MLIR territory).

tblah updated this revision to Diff 506544.Mon, Mar 20, 4:55 AM
tblah marked 2 inline comments as done.

Thanks for the review. Changes are as follows:

  • Move createHLFIRToFIRPassPipeline before #if !defined(FLANG_EXCCLUDE_CODEGEN
  • Only canonicalize HLFIR (creating hlfir.matmul_transpose) if we are optimizing for speed
  • Unconditionally run HLFIR passes
  • Add tests
awarzynski added inline comments.Mon, Mar 20, 6:55 AM

It looks like these passes are run unconditionally - what's the -flang-experimental-hlfir flag for then?

tblah added inline comments.Mon, Mar 20, 7:19 AM

Adding this to bbc will have to wait until after -emit-fir and -emit-hlfir are different flags. Otherwise hlfir ops will be lowered to fir, breaking some tests (and presumably people's workflows).


So far as I know, there should be no loss to precision from implementing it as a combined operation. Memory usage should be reduced as we need one fewer temporary.

If static linking is used, including MATMUL_TRANSPOSE in the runtime library will increase code size (so long as both matmul and transpose are also called elsewhere). I haven't measured this, but I wouldn't expect this to be a large change relative to the size of a real world application.

If dynamic linking is used, whether or not this pass runs, MATMUL_TRANSPOSE will make the runtime library a little larger (there are a lot of template instantiations, but MATMUL_TRANSPOSE is only one of many similar functions so the effect as a proportion of the whole shouldn't be much).

But I'll set the canonicalization pass to only run when we are optimizing for speed. Later canonicalisation passes (after createLowerHLFIRIntrinsicsPass) won't find any hlfir.matmul operations to canonicalise and so won't create a hlfir.matmul_transpose operation.


Okay I'll run the HLFIR passes unconditionally.

I made -emit-fir output HLFIR to match what bbc already does, but I can see the usefulness of having both -emit-fir and -emit-hlfir. I won't do it in this patch but I will get around to this at some point (but feel free to jump in sooner if you need the flag for something).


Yes. @vzakhari requested the passes should run unconditionally.

-flang-experimental-hlfir controls whether the HLFIR lowering is used.

awarzynski added inline comments.Mon, Mar 20, 8:26 AM

Ok, so so far you have updated the default pass pipelines and that's what you are testing here. However, this patch is about -flang-experimental-hlfir - can you add a test for this flag?

Please address @awarzynski's comment about the test.


Okay! Thank you for considering it!


Thank you!

tblah updated this revision to Diff 507026.Tue, Mar 21, 9:58 AM

Added a test

awarzynski added inline comments.Tue, Mar 21, 10:12 AM

Could you also add a RUN line like this:

! RUN: %flang_fc1 -emit-fir -o - %s | FileCheck %s --prefix="NO-HLFIR"

! CHECK-NOT: hlfir
tblah updated this revision to Diff 507321.Wed, Mar 22, 5:17 AM

Thanks for the suggestion

  • Added a test to check that hlfir is not output without the flag
  • clang-formatted
awarzynski accepted this revision.Wed, Mar 22, 6:10 AM

LGTM, thanks!


[ultra nit] I would separate with an empty line for better readability.

This revision is now accepted and ready to land.Wed, Mar 22, 6:10 AM
This revision was landed with ongoing or failed builds.Wed, Mar 22, 6:38 AM
This revision was automatically updated to reflect the committed changes.