This is an archive of the discontinued LLVM Phabricator instance.

[flang][driver] Add support for -mmlir
ClosedPublic

Authored by awarzynski on Apr 7 2022, 3:31 AM.

Details

Summary

The semantic of -mmlir are identical to -mllvm. The only notable difference is that -mmlir options should be forwarded to MLIR rather than LLVM.

Diff Detail

Event Timeline

awarzynski created this revision.Apr 7 2022, 3:31 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
awarzynski requested review of this revision.Apr 7 2022, 3:31 AM
awarzynski edited the summary of this revision. (Show Details)Apr 7 2022, 3:33 AM
rovka added a comment.Apr 7 2022, 5:00 AM

I don't know the command line library that well, so I have this curiosity: what happens if LLVM and MLIR have 2 different options with the same name? Do we get a compile time error? Or is there a risk that someone might -mllvm -XYZ and it would end up in MLIR's XYZ option instead, because we're processing the MLIR options after the LLVM ones?

flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
15

Nit: Should these come after the llvm/ headers? (So it's alphabetical except for flang, which may go first)

flang/test/Driver/mllvm_vs_mmlir.f90
17

Is this the most llvm-ish option we have? I'm concerned that MLIR might also decide to add an option that sounds like this (and for sure -print-ir-before-all is mentioned in the MLIR docs).

rovka added inline comments.Apr 7 2022, 5:02 AM
flang/test/Driver/mllvm_vs_mmlir.f90
17

Is this the most llvm-ish option we have? I'm concerned that MLIR might also decide to add an option that sounds like this (and for sure -print-ir-before-all is mentioned in the MLIR docs).

Right, I think that might be why the premerge tests are failing...

awarzynski updated this revision to Diff 421168.Apr 7 2022, 5:28 AM
awarzynski edited the summary of this revision. (Show Details)

Fix pre-commit CI

I don't know the command line library that well, so I have this curiosity: what happens if LLVM and MLIR have 2 different options with the same name? Do we get a compile time error?

The llvm::cl API uses global variables. So, if a variable is defined twice, you will get a compilation error. I will advertise this before merging so that folks from LLVM and MLIR are aware of this change. I'm hoping that they won't mind having to use distinct variable names for CL options in MLIR and LLVM.

Or is there a risk that someone might -mllvm -XYZ and it would end up in MLIR's XYZ option instead, because we're processing the MLIR options after the LLVM ones?

Note that -mllvm options are saved in llvmArgs and -mmlir options are saved in mlirArgs (this is taken care of in "CompilerInvocation.cpp"). This should guarantee that the right option set is used.

flang/test/Driver/mllvm_vs_mmlir.f90
17

Is this the most llvm-ish option we have?

Sadly, most LLVM options have rather generic names that could at some point be added in MLIR. Perhaps you'll spot something more suitable:

USAGE: flang (LLVM option parsing) [options]

OPTIONS:

Color Options:

  --color                                           - Use colors in output (default=autodetect)

General options:

  --aarch64-neon-syntax=<value>                     - Choose style of NEON code to emit from AArch64 backend:
    =generic                                        -   Emit generic NEON assembly
    =apple                                          -   Emit Apple-style NEON assembly
  --aarch64-use-aa                                  - Enable the use of AA during codegen.
  --abort-on-max-devirt-iterations-reached          - Abort when the max iterations for devirtualization CGSCC repeat pass is reached
  --allow-ginsert-as-artifact                       - Allow G_INSERT to be considered an artifact. Hack around AMDGPU test infinite loops.
  --always-execute-loop-body                        - force the body of a loop to execute at least once
  --array-constructor-initial-buffer-size=<uint>    - set the incremental array construction buffer size (default=32)
  --cfg-hide-cold-paths=<number>                    - Hide blocks with relative frequency below the given value
  --cfg-hide-deoptimize-paths                       -
  --cfg-hide-unreachable-paths                      -
  --cost-kind=<value>                               - Target cost kind
    =throughput                                     -   Reciprocal throughput
    =latency                                        -   Instruction latency
    =code-size                                      -   Code size
    =size-latency                                   -   Code size and latency
  --debugify-level=<value>                          - Kind of debug info to add
    =locations                                      -   Locations only
    =location+variables                             -   Locations and Variables
  --debugify-quiet                                  - Suppress verbose debugify output
  --default-kinds=<default-kind-string>             - string to set default kind values
  --disable-i2p-p2i-opt                             - Disables inttoptr/ptrtoint roundtrip optimization
  --dot-cfg-mssa=<file name for generated dot file> - file name for generated dot file
  --enable-cse-in-irtranslator                      - Should enable CSE in irtranslator
  --enable-cse-in-legalizer                         - Should enable CSE in Legalizer
  --enable-gvn-memdep                               -
  --enable-load-in-loop-pre                         -
  --enable-load-pre                                 -
  --enable-loop-simplifycfg-term-folding            -
  --enable-name-compression                         - Enable name/filename string compression
  --enable-split-backedge-in-load-pre               -
  --experimental-debug-variable-locations           - Use experimental new value-tracking variable locations
  --fdebug-dump-pre-fir                             - dump the Pre-FIR tree prior to FIR generation
  --fs-profile-debug-bw-threshold=<uint>            - Only show debug message if the source branch weight is greater  than this value.
  --fs-profile-debug-prob-diff-threshold=<uint>     - Only show debug message if the branch probility is greater than this value (in percentage).
  --gen-array-coor                                  - in lowering create ArrayCoorOp instead of CoordinateOp
  --generate-merged-base-profiles                   - When generating nested context-sensitive profiles, always generate extra base profile for function with all its context profiles merged into it.
  --inline-all                                      - aggressively inline everything
  --instcombine-code-sinking                        - Enable code sinking
  --instcombine-guard-widening-window=<uint>        - How wide an instruction window to bypass looking for another guard
  --instcombine-max-iterations=<uint>               - Limit the maximum number of instruction combining iterations
  --instcombine-max-num-phis=<uint>                 - Maximum number phis to handle in intptr/ptrint folding
  --instcombine-max-sink-users=<uint>               - Maximum number of undroppable users for instruction sinking
  --instcombine-maxarray-size=<uint>                - Maximum array size considered when doing a combine
  --instcombine-negator-enabled                     - Should we attempt to sink negations?
  --instcombine-negator-max-depth=<uint>            - What is the maximal lookup depth when trying to check for viability of negation sinking.
  --kind-mapping=<kind-mapping-string>              - kind mapping string to set kind precision
  --length-to-hash-string-literal=<ulong>           - string literals that exceed this length will use a hash value as their symbol name
  --main-entry-name=<string>                        - override the name of the default PROGRAM entry (may be helpful for using other runtimes)
  --math-runtime=<value>                            - Select math runtime version:
    =fast                                           -   use pgmath fast runtime
    =relaxed                                        -   use pgmath relaxed runtime
    =precise                                        -   use pgmath precise runtime
    =llvm                                           -   only use LLVM intrinsics (may be incomplete)
  --matrix-default-layout=<value>                   - Sets the default matrix layout
    =column-major                                   -   Use column-major layout
    =row-major                                      -   Use row-major layout
  --mir-strip-debugify-only                         - Should mir-strip-debug only strip debug info from debugified modules by default
  --no-discriminators                               - Disable generation of discriminator information.
  --non-recursive-procedures                        - Make procedures non-recursive by default. This was the default for all Fortran standards prior to 2018.
  --opaque-pointers                                 - Use opaque pointers
  --outline-intrinsics                              - Lower all intrinsic procedure implementation in their own functions
  --safepoint-ir-verifier-print-only                -
  --sample-profile-check-record-coverage=<N>        - Emit a warning if less than N% of records in the input profile are matched to the IR.
  --sample-profile-check-sample-coverage=<N>        - Emit a warning if less than N% of samples in the input profile are matched to the IR.
  --sample-profile-max-propagate-iterations=<uint>  - Maximum number of iterations to go through when propagating sample block/edge weights through the CFG.
  --use-alloc-runtime                               - Lower allocations to fortran runtime calls
  --use-desc-for-alloc                              - Always use descriptors for POINTER and ALLOCATABLE
  --verify-legalizer-debug-locs=<value>             - Verify that debug locations are handled
    =none                                           -   No verification
    =legalizations                                  -   Verify legalizations
    =legalizations+artifactcombiners                -   Verify legalizations and artifact combines
  --verify-region-info                              - Verify region info (time consuming)

Generic Options:

  --help                                            - Display available options (--help-hidden for more)
  --help-list                                       - Display list of available options (--help-list-hidden for more)
  --version                                         - Display the version of this program
17

Right, I think that might be why the premerge tests are failing...

Doh, I updated the test before sending this, but forgot to re-run it :( Should be fixed now.

awarzynski updated this revision to Diff 421223.Apr 7 2022, 8:19 AM

Make sure the MLIR CL options are "applied"

rriddle added inline comments.Apr 7 2022, 8:27 AM
flang/test/Driver/mllvm_vs_mmlir.f90
17

I'd be alright with a patch to MLIR that prefixed the pass manager options with mlir-. The rest of our options (that are provided by libraries) should already be prefixed with that AFAIK.

awarzynski added inline comments.Apr 7 2022, 9:49 AM
flang/test/Driver/mllvm_vs_mmlir.f90
17

Sounds like a plan, thanks for the suggestion!

mehdi_amini added a comment.EditedApr 7 2022, 10:55 AM

-mmlir is fine with me, but note that MLIR has much less global options than LLVM: you will only get access to context and passmanager options and not individual passes flags. That's not a criticism :)
(that's what makes it not likely to have conflicts by the way: the set of MLIR option is well identified)

-mmlir is fine with me, but note that MLIR has much less global options than LLVM: you will only get access to context and passmanager options and not individual passes flags. That's not a criticism :)
(that's what makes it not likely to have conflicts by the way: the set of MLIR option is well identified)

Thanks for taking a look! All this should be fine - the main rationale for this patch is to enable MLIR options in flang-new so that tests using tco or bbc (and which use MLIR options) can be ported to use flang-new. We were discussing this recently in https://reviews.llvm.org/D121171.

awarzynski added inline comments.Apr 11 2022, 4:24 AM
flang/test/Driver/mllvm_vs_mmlir.f90
17
rovka added inline comments.Apr 12 2022, 1:27 AM
flang/test/Driver/mllvm_vs_mmlir.f90
17

Should we add this as a parent then?

awarzynski added inline comments.Apr 12 2022, 2:43 AM
flang/test/Driver/mllvm_vs_mmlir.f90
17

I've just merged it, so can re-base this patch instead.

I did a bit of digging and realised that MLIR llvm::cl options are lazily constructed on demand (see the definition of options in PassManagerOptions.cpp). This means that:

  • Flang and LLVM global options are always visible and displayed when passing -mllvm -help,
  • MLIR global options are only visible when explicitly initialised and displayed when passing -mmlir -help.

Since Flang and LLVM global options are always visible, llvm::cl will display them alongside MLIR options when using -mmlir -help. In other words, -mmlir -help is a superset of --mllvm -help. This is not ideal, but we'd need to refactor all option definitions in Flang and LLVM to improve this. I suggesting leaving this for later.

Rebase on top of main, re-fine the test

Make sure that the test file correctly takes into account that -mmlir -help is a superset of -mllvm -help.

rovka accepted this revision.Apr 13 2022, 2:24 AM

LGTM, thanks!

This revision is now accepted and ready to land.Apr 13 2022, 2:24 AM
awarzynski added inline comments.Apr 13 2022, 3:42 AM
flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
15

Thanks, I will fix this before merging!

MaskRay added inline comments.Apr 13 2022, 8:14 PM
clang/include/clang/Driver/Options.td
3263
This revision was automatically updated to reflect the committed changes.
awarzynski added inline comments.Apr 14 2022, 2:41 AM
clang/include/clang/Driver/Options.td
3263

Thanks, included in the final version!