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.

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

Port Classic Flang to LLVM 14 #106

Merged
merged 31 commits into from
Jun 7, 2022

Conversation

bryanpkc
Copy link
Collaborator

This PR ports Classic Flang-related changes from flang-13.0.0-20220325 to llvmorg-14.0.1, on which our new release_14x branch will be based.

I have squashed some commits together to resolve merge conflicts and make them buildable; in such cases, the original commits are cited. In some cases, I have undone the squashes performed by @abrahamtovarmob and separated commits that have clearly different purposes. Some upstream commits which were backported to release_13x did not need to be ported to this branch (89065d4, b4723b9, 0254365).

A commit was added on top of the branch to enable GitHub builds of the release_14x branch.

Copy link
Collaborator

@RichBarton-Arm RichBarton-Arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have recently done this exercise downstream so I took a look. I left a few comments in passing for minor things.

It looks like you need to cherry pick across my three recent PR #100 , #102 and #104 .

When doing this downstream, I found a regression in the way classic flang handles preprocessing with -save-temps (and some other not important options). flang -E -save-temps file.F95 causes the preprocessor to be run twice. I have a fix so perhaps it is easiest if I make a separate PR and we can commit it over the top of this one?

clang/include/clang/Driver/Options.td Outdated Show resolved Hide resolved
clang/include/clang/Driver/Options.td Outdated Show resolved Hide resolved
HelpText<"Process source files in fixed form">;
def ffree_form : Flag<["-"], "ffree-form">, Group<f_Group>,
#ifdef ENABLE_CLASSIC_FLANG
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned about using ENABLE_CLASSIC_FLANG to guard additions to Options.td when it does not also guard alterations to Options.td. That seems confusing to me. I don't think there is any chance of an unintended downside to having classic flang options defined in the compiled opt table when classic flang is not included? They simply won't be used by the rest of the code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you. Simply porting the old inconsistent guards to LLVM 14 is easy, but we really ought to make these things consistent. I will take a deeper look at the uses of ENABLE_CLASSIC_FLANG in this file. This is a good opportunity to make the file compatible with both LLVM Flang and Classic Flang (we are playing with both downstream).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated the uses of the macro in the file so that the conflicting Fortran options are better guarded.

}

#ifdef ENABLE_CLASSIC_FLANG
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although this is quite nice and clear, despite my comment above about not liking ENABLE_CLASSIC_FLANG use like this....

Perhaps I could be persuaded, but IMO we should just modify this file for classic flang without using ENABLE_CLASSIC_FLANG. What do others think?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the record, I am very happy with the new approach that lets classic flang sit next to llvm flang wrt Options.td.
Sterling work @bryanpkc

clang/lib/Driver/Driver.cpp Show resolved Hide resolved
@bryanpkc
Copy link
Collaborator Author

I added a patch to revert an ancient change in LoopVectorize.cpp, which was causing an AArch64 test to fail. This probably needs a review by @vjayathirtha-nv or someone more familiar with the motivation of the original patch.

@bryanpkc
Copy link
Collaborator Author

@RichBarton-Arm Thanks for the review; I'll address your concerns soon.

@bryanpkc
Copy link
Collaborator Author

When doing this downstream, I found a regression in the way classic flang handles preprocessing with -save-temps (and some other not important options). flang -E -save-temps file.F95 causes the preprocessor to be run twice. I have a fix so perhaps it is easiest if I make a separate PR and we can commit it over the top of this one?

That sounds like a good idea. In this PR I was only porting the basic patches from flang-13.0.0-20220325 so there would have been a follow-up PR to cherry-pick other more recent patches. I also didn't notice the double preprocessing problem in our downstream compiler previously.

@vjayathirtha-nv
Copy link
Contributor

I added a patch to revert an ancient change in LoopVectorize.cpp, which was causing an AArch64 test to fail. This probably needs a review by @vjayathirtha-nv or someone more familiar with the motivation of the original patch.

Hi @bryanpkc, this comment describes the context for this change. However I'm not in charge of maintaining classic flang anymore, maybe @sscalpone or @schweitzpgi can help you further.

CommonCmdArgs.push_back("120");
CommonCmdArgs.push_back("0x4000");
if (!GDwarfArg) // -g without -gdwarf-X produces default (DWARFv4)
CommonCmdArgs.push_back("0x1000000");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clang has moved to DWARF 5 by default, should this be updated as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The option -Mx,120,0x2000000 should enable DWARF 5, but I am not sure how well it is supported; more thorough testing and perhaps some additional fixes might be needed. I will create an issue to track this work.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be very afraid to change default here. Better the devil you know.

@bryanpkc
Copy link
Collaborator Author

bryanpkc commented May 3, 2022

flang-compiler/flang#1242 will fix the test failures seen in the "Build and test Flang" jobs in this PR.

Copy link
Collaborator

@RichBarton-Arm RichBarton-Arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great Bryan! I like the solution to Options.td preprocessing now, thanks for taking time to do that.

I notice that there are a number of places in the code where the classic flang changes are not guarded with ENABLE_CLASSIC_FLANG Some of those are harmless, but it looks to my eye like some of them need guarding after your changes to Options.td preprocessing. Worth a look before we approve.

@@ -525,7 +525,7 @@ class InternalDriverOpt : Group<internal_driver_Group>,
Flags<[NoXarchOption, HelpHidden]>;
def driver_mode : Joined<["--"], "driver-mode=">, Group<internal_driver_Group>,
Flags<[CoreOption, NoXarchOption, HelpHidden]>,
HelpText<"Set the driver mode to either 'gcc', 'g++', 'cpp', or 'cl'">;
HelpText<"Set the driver mode to one of: 'gcc', 'g++', 'cpp', 'cl', or 'fortran'">;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like an unnecessary bit of delta in classic flang that could be removed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I think this change should be upstreamed, but 'fortran' should be replaced with 'flang'.

clang/include/clang/Driver/Options.td Show resolved Hide resolved
clang/include/clang/Driver/Options.td Show resolved Hide resolved
}

#ifdef ENABLE_CLASSIC_FLANG
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the record, I am very happy with the new approach that lets classic flang sit next to llvm flang wrt Options.td.
Sterling work @bryanpkc

clang/include/clang/Driver/ToolChain.h Show resolved Hide resolved
clang/lib/CodeGen/CGDebugInfo.cpp Show resolved Hide resolved
clang/lib/Driver/CMakeLists.txt Outdated Show resolved Hide resolved
Args.AddLastArg(CmdArgs, options::OPT_fveclib);
#endif

std::string PassRemarkVal(""), PassRemarkOpt("");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this whole block also needs an include guard as it uses OPT_Minfo_EQ and friends? So I think this branch currently wouldn't build with -DLLVM_ENABLE_CLASSIC_FLANG=Off

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird also how this block needs to be in the Clang driver and can't just be in ClassicFlang.cpp

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have guarded the whole block with ENABLE_CLASSIC_FLANG. I will create a follow-up issue to investigate moving this code to a more appropriate location.

CommonCmdArgs.push_back("120");
CommonCmdArgs.push_back("0x4000");
if (!GDwarfArg) // -g without -gdwarf-X produces default (DWARFv4)
CommonCmdArgs.push_back("0x1000000");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be very afraid to change default here. Better the devil you know.

clang/lib/Driver/ToolChains/CommonArgs.cpp Show resolved Hide resolved
Cherry-picked cabccf8f2d00daf1d740b18a1c80e9b5f4593f60.
@bryanpkc
Copy link
Collaborator Author

bryanpkc commented May 5, 2022

flang-compiler/flang#1244 is needed to fix several AArch64 Flang test cases.

@bryanpkc
Copy link
Collaborator Author

@RichBarton-Arm Could you take a look at this again?

Copy link
Collaborator

@RichBarton-Arm RichBarton-Arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two comments, but from the clang/driver side I think this merge is good to go with those two addressed. It would be good for someone to also look over the Debug changes here to check they are all legit.

My two comments:

  1. It looks like this commit is not present in this port, but I expected it to be: 89065d4 @pawosm-arm can confirm if that's right.
  2. I left one inline comment also

//===----------------------------------------------------------------------===//

#ifndef LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_Flang_H
#define LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_Flang_H
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be ..._ClassicFlang_H ?

Copy link
Collaborator

@pawosm-arm pawosm-arm May 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commit mentioned by Richard should be in this port.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commit is not in this port because it is already part of the baseline llvmorg-14.0.1 (commit 26f5643).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The macro has been fixed.

bryanpkc and others added 12 commits May 20, 2022 16:11
This is a combination of cherry-picking the following patches:

Commit 82b38d2:

    [ClassicFlang] Port release_90 changes from flang-compiler/llvm

    Cherry-picked commit 2085211cfcca70411dc63f0d08763facc8a02090 by Eric Schweitz,
    resolved merge conflicts, fixed build failures (e.g. adapted CGDebugInfo.cpp to
    the new API), and fixed the DIGlobalVariable unit tests, which have been broken
    since commit edfad65eebdf045b050f37380b6b61d673513982.

Commit 885dd87e5fdc:

    [DebugInfo] Removal of DIFortranArrayType and DIFortranSubrange

    These extensions are no more required after merge of below PR.

Commit 5c9b2e0867d5:

    Modification to incorporate comments from @bryanpkc.

Commit 822de2c:

    [AsmPrinter] Fix redundant names and types

    A bug was introduced in 82b38d2 while
    cherry-picking a DIGlobalVariable-related patch.

Commit 45a70a8:

    Port driver changes from release/11x

    Cherry-picked c51f89679135f84675f492d560ec5535c2000cfe by Varun Jayathirtha
    from release_90, and resolved merge conflicts.

    To avoid conflicts with the new Flang, lib/Driver/ToolChains/Flang.{cpp,h}
    have been renamed to ClassicFlang.{cpp,h}, and the ENABLE_CLASSIC_FLANG macro
    is introduced to select which incarnation of Flang to build. The macro is set
    by running CMake with -DLLVM_ENABLE_CLASSIC_FLANG.

    After merge with LLVM 11: Move flang options to the end of the definitions list.

Commit a9a8036:

    Port Classic Flang to LLVM 13

    File changes to TargetLibraryInfo and DebugLocEntry to adapt to the code from
    release/13.x and make it work. Comment out the changes due a segmentation
    fault, code need to be reviewed properly once all commits are in

Commit fe989b7:

    Fix -fveclib=PGMATH

    The use of #ifdef in include/clang/Driver/Options.td was incorrect and
    unsupported. As a result -fveclib=PGMATH was silently ignored, and
    in LLVM 12, it causes the invocation to fail. This patch unguards the
    option so that it is parsed correctly, but lets the FLANG_LLVM_EXTENSIONS
    macro continue to toggle the feature.

Commit 7c224ae:

    Fix use of classic Flang as preprocessor

Commit 8403c83:

    Merge FLANG_LLVM_EXTENSIONS macro into ENABLE_CLASSIC_FLANG

Commit 486741e:

    Fix test failures when in classic Flang mode

    Add a new lit feature tag "classic_flang" to select which tests can or cannot be
    run when the driver is built for classic Flang. Handle LLVM_ENABLE_CLASSIC_FLANG
    in llvm/cmake/modules/HandleLLVMOptions.cmake instead of clang/CMakeLists.txt so
    that macro works in both clang and llvm.

Commit a10f592:

    Port Classic Flang to LLVM 13

    LLVM port from release_12x to release_13x, changes done in order to
    make project able to be build.

Commit d385321 (partial):

    Change to Options.td in order to add the correct invocation for
    ffixed_line_length_VALUE.
Make TableGen respect the ENABLE_CLASSIC_FLANG macro and better separate the
driver options that are mutually exclusive. When the macro is defined, do not
build LLVM Flang at all.
…lang-compiler#92)

* Support for -gdwarf-5 option in Flang driver.

Summary:
  FLANG driver doesnt pass -gdwarf-4/5 to flang1 in form of xbits,
while it passes for -gdwarf-2/3
   -gdwarf-2 => -x 120 0x200
   -gdwarf-3 => -x 120 0x4000

Due to this -gdwarf-5 is never honored and default option -gdwarf-4
is taken.
    # flang -gdwarf-5 test.f90
    # llvm-dwarfdump a.out | grep version
    0x00000000: Compile Unit: length = 0x0000008e version = 0x0004

  Now 0x1000000/0x2000000 will be passed for -gdwarf-4/5
   -gdwarf-4 => -x 120 0x1000000
   -gdwarf-5 => -x 120 0x2000000

Testing:
  - GNU gdb fortran testsuite
  - check-llvm
  - check-debuginfo

* Flang doenst choose correct dwarf version when multiple -g/-gdwarfN mentioned

 Summary:
When multiple -g/-gdwarfN options are passed together at compile time,
flang chooses the least one. Clang/gfortran etc choose the last one.

-gdwarf-5 -gdwarf-3 => flang chooses 5 while clang/gfortran choose 3
-gdwarf-5 -g => flang choses the default while clang/gfortran choose 5

  Testing:
- check-llvm
- check-debuginfo

* Default dwarf version should be 4 for -g with flang

Currently flang dumps dwarf version 2 for -g and 4 for absence of -g

-------------------------
$ flang my.f90
$ llvm-dwarfdump a.out | grep version
0x00000000: Compile Unit: length = 0x0000003d version = 0x0004 abbr_offset = 0x0000 addr_size = 0x08 (next unit at 0x00000041)

$ flang -g my.f90
$ llvm-dwarfdump a.out | grep version
0x00000000: Compile Unit: length = 0x00000047 version = 0x0002 abbr_offset = 0x0000 addr_size = 0x08 (next unit at 0x0000004b)
-------------------------

It should be 4 for -g as it is the case with clang.
This commit is motivated by reducing the merge burden by shrinking the
diff between llvm upstream and classic-flang-llvm-project.

Outside of Flang, Fortran code is fed through the Compile phase, and
the appropriate tooling is picked up through ToolChain::SelectTool.

Classic Flang introduced a FortranFrontend, but these days this seems
unnecessary. Fortran can go through the same machinery as everything else.

* Use the Preprocess phase to preprocess Fortran code. This phase is
  always combined with the Compile phase.

* Use the Compile phase to lower Fortran code to LLVM IR, and use the
  Backend phase to compile and optimize the IR. These phases are never
  combined.

* Remove FortranFrontendJobClass.

* Remove FortranFrontend tool (instead it's just the Flang tool, which
  in Classic Flang mode is Classic Flang).

* Update tests which inspect the output of the Classic Flang tooling,
  and ensures that the driver does the right thing for various types of
  inputs.

Based on a patch from Peter Waller <peter.waller@arm.com>.
This commit also squashes a number of patches from the release_12x branch:

  commit 8eaf5ef
  Author: Michal Paszta <michal.paszta@mobica.com>
  Date:   Wed Nov 4 21:08:29 2020 +0100

      Github Actions added to pre-compile artifacts for flang

  commit d4e2d22
  Author: michalpasztamobica <michal.paszta@mobica.com>
  Date:   Fri Jan 22 10:39:07 2021 +0100

      Github Actions to build release_11x and release_12x

      release_100 is still built as it was.
      Artifacts names include the branch name.

  commit bfa53aa
  Author: Michał Janiszewski <janisozaur@users.noreply.github.com>
  Date:   Wed Feb 17 14:41:51 2021 +0100

      Enable ccache for GitHub Actions

  commit 04e9947
  Author: michalpasztamobica <michal.paszta@mobica.com>
  Date:   Wed Mar 17 20:27:04 2021 +0100

      Allow full path to flang1 and flang2 in classic flang tests.

  commit 5b60137
  Author: michalpasztamobica <michal.paszta@mobica.com>
  Date:   Fri Mar 19 10:41:22 2021 +0100

      Remove unnecessary caret from path in classic-flang test

  commit 52f2e34
  Author: michalpasztamobica <michal.paszta@mobica.com>
  Date:   Mon Apr 19 09:54:05 2021 +0200

      Remove the builds for gcc-9 and llvm-9

  commit f9f54d2
  Author: michalpasztamobica <michal.paszta@mobica.com>
  Date:   Thu Apr 29 16:40:35 2021 +0200

      Build and test Flang project on PRs

  commit 89ffbbd
  Author: michalpasztamobica <michal.paszta@mobica.com>
  Date:   Thu May 6 10:19:41 2021 +0200

      Prepare llvm and clang for ARM64 build of Flang

      Run Dockerfile script on Github's server to prepare a docker image with
      llvm and clang built and installed for Flang to use when testing code.
For the LLVM 14 port, pass "140" as the driver version to flang2.
Signed-off-by: Itay Bookstein <itay.bookstein@nextsilicon.com>
* -Wsuggest-override for ClassicFlangMacroBuilder::defineMacro
* -Wrange-loop-construct when iterating over llvm::opt::Arg::getValues

Signed-off-by: Itay Bookstein <itay.bookstein@nextsilicon.com>
Changes to LLVM release_13x to allow it to be built by GitHub.

This is a combination of 4 commits by Abraham Tover from the release_13x branch
which had duplicate commit messages and overlapping content:

    c53d8f8 LLVM release_13x CI changes
    83c659a Fix release_13x CI dockerfile
    5e6e4cb Fix release_13x CI dockerfile
    fc42794 LLVM release_13x CI changes
-emit-flang-llvm instructs flang to stop after flang2 and dump the LLVM IR.

Can be useful for debugging and also would be a useful option for testing flang
output more accurately.

Signed-off-by: Richard Barton <richard.barton@arm.com>
Add %flang as a tool substitution available in lit tests. This apes the way
%clang is defined and adds a $FLANG override in a similar vein.

To avoid this being dead code, add a single test to check the flang driver is
reporting the correct phases when running under various phase control options.

Signed-off-by: Richard Barton <richard.barton@arm.com>
Pass LLVM target features (-mattr strings) to flang to embed in generated
.ll files. For normal compilation this won't make much difference as the
attributes are passed to clang after flang2 and can be applied there but this
is crucial to enable LTO with flang as clang will not apply the attributes
when building the flang2 output. libLTO will need to read these out of the
object files to apply them.

Signed-off-by: Richard Barton <richard.barton@arm.com>
Use existing clang support for uniquifying target feature lists to pass
a unique list through to flang.
Signed-off-by: Paul Osmialowski <pawel.osmialowski@arm.com>
Also fix Dockerfile to install missing updates, and use apt instead of apt-get.
Install python-distutils for timeout handling.
Signed-off-by: Paul Osmialowski <pawel.osmialowski@arm.com>
This patch partially reverts the following commit from an early version of
Classic Flang LLVM:

flang-compiler/llvm@6866909

When the LoopVectorize.cpp change in that commit is ported to LLVM 14, it causes
a failure in the following test:

llvm/test/Transforms/LoopVectorize/AArch64/smallest-and-widest-types.ll

Reverting the change allows all tests to run cleanly.
Version screen has been erroneously reporting as flang-new since LLVM 12
These CMake variables are used to set python variables in the lit
config. As such, they need to bet set with a valid CMake boolean value
that is also a valid python boolean value (1, True, etc.) and not one
that is not (ON, NO, etc.) to work. This is fragile.

Call the LLVM cmake helper function on these two downstream CMake
Signed-off-by: Richard Barton <richard.barton@arm.com>
@kiranchandramohan
Copy link
Collaborator

ping @shivaramaarao

@bryanpkc bryanpkc merged commit b33a874 into flang-compiler:release_14x Jun 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.