Port release_90 patches to LLVM 10 monorepo

[bryanpkc]

This pull request cherry-picks all relevant patches from the release_90 branches in https://github.com/flang-compiler/flang-driver and https://github.com/flang-compiler/llvm, adapts them to fit the monorepo layout, and adjusts the Classic Flang code to let it coexist with the new Flang. To build the code, run CMake with -DLLVM_ENABLE_CLASSIC_FLANG=on.

See https://github.com/Huawei-PTLab/classic-flang-llvm/wiki for an example build script.

[kiranchandramohan]

@bryanpkc Is the CI (results above) running from your repository? Is it expected to fail?

[bryanpkc]

@kiranchandramohan I think those CI jobs came from the GitHub Actions that are configured for your repository:

https://github.com/flang-compiler/classic-flang-llvm-project/actions

These CI jobs are probably incorrect or irrelevant. For our purpose, a useful CI job should build llvm-project with -DLLVM_ENABLE_CLASSIC_FLANG=ON.

[bryanpkc]

I found a bug in my branch; I will push a fix shortly.

[RichBarton-Arm]

Classic flang had no pre-commit CI on any repositories before so these were not pre-existing CI configs. Presumably you made a fork of llvm/llvm-project to make flang-compiler/classic-flang-llvm-project? Is it possible that these were somehow inherited from llvm/llvm-project when the fork was made? From: Bryan Chan <notifications@github.com> Sent: 21 August, 2020 23:07 To: flang-compiler/classic-flang-llvm-project <classic-flang-llvm-project@noreply.github.com> Cc: Richard Barton <Richard.Barton@arm.com>; Review requested <review_requested@noreply.github.com> Subject: Re: [flang-compiler/classic-flang-llvm-project] Port release_90 patches to LLVM 10 monorepo (#1) I found a bug in my branch; I will push a fix shortly. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub<#1 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AKKGCW5XZ7K6IAZA5E2CKCTSB3VXRANCNFSM4QEAMWHQ>. IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[bryanpkc]

Classic flang had no pre-commit CI on any repositories before so these were not pre-existing CI configs. Presumably you made a fork of llvm/llvm-project to make flang-compiler/classic-flang-llvm-project? Is it possible that these were somehow inherited from llvm/llvm-project when the fork was made?

The master branch in classic-flang-llvm-project does not contain a .github/workflows/ directory, however, the release_100 branch does. The directory is inherited from the release/10.x branch of llvm/llvm-project; I did not add the directory or configure any GitHub Action through the web UI.

It's not a bad idea to have some CI jobs to ensure that this repo is functionally correct, and especially can still build Clang normally.

[bryanpkc]

I have fixed this pull request to allow normal Clang builds and regression tests to succeed, except for the two tests mentioned in issue #2. Ideally we should fix those tests too so that we have a clean branch.

This port adds a USE_CLASSIC_FLANG macro to Clang (a28a4fa), in order to resolve some conflicts between the classic Flang modifications and the F18-related changes in release/10.x. Please let me know if that is acceptable. Most other commits are straightforward cherry-pick of the corresponding patches from flang-compiler/flang-driver and flang-compiler/llvm.

[kiranchandramohan]

Was able to build flang with this monorepo using the build instructions provided. Copying the build instructions below (has trivial modifications to work with this monorepo). All tests in flang passes on Aarch64, two tests in llvm debug fails as @bryanpkc mentions.

I will go through the code next.

#!/bin/bash

buildtype=Release
installdir=$PWD/install

if [ ! -d classic-flang-llvm-project ]; then
  git clone https://github.com/flang-compiler/classic-flang-llvm-project.git
  pushd classic-flang-llvm-project
  git fetch origin pull/1/head:mono10
  git checkout mono10
  popd
fi
if [ ! -d flang ]; then
  git clone https://github.com/flang-compiler/flang
fi

mkdir -p build/llvm
pushd build/llvm
cmake -G Ninja \
  -DCMAKE_BUILD_TYPE=$buildtype \
  -DCMAKE_INSTALL_PREFIX=$installdir \
  -DCMAKE_C_COMPILER=gcc \
  -DCMAKE_CXX_COMPILER=g++ \
  -DLLVM_ENABLE_PROJECTS="clang;openmp" \
  -DLLVM_ENABLE_CLASSIC_FLANG=on \
  -DLLVM_ENABLE_ASSERTIONS=on \
  -DLLVM_OPTIMIZED_TABLEGEN=on \
  -DLLVM_TARGETS_TO_BUILD="X86;AArch64" \
  ../../classic-flang-llvm-project/llvm || exit 1
nice -n 10 ninja install || exit 1
popd

if ! llvm_lit="$(realpath --canonicalize-existing $PWD/build/llvm/bin/llvm-lit 2>/dev/null)" || \
    [ ! -x "$llvm_lit" ]; then
  echo "no usable llvm-lit"
  exit 1
fi

mkdir -p build/libpgmath
pushd build/libpgmath
cmake -G Ninja \
  -DCMAKE_BUILD_TYPE=$buildtype \
  -DCMAKE_INSTALL_PREFIX=$installdir \
  -DCMAKE_C_COMPILER=gcc \
  -DCMAKE_CXX_COMPILER=g++ \
  -DLLVM_CONFIG=$installdir/bin/llvm-config \
  -DLLVM_TARGETS_TO_BUILD="X86;AArch64" \
  -DLIBPGMATH_LLVM_LIT_EXECUTABLE="$llvm_lit" \
  ../../flang/runtime/libpgmath || exit 1
nice -n 10 ninja check-libpgmath install || exit 1
popd

mkdir -p build/flang
pushd build/flang
cmake -Wno-dev -G 'Unix Makefiles' \
  -DCMAKE_BUILD_TYPE=$buildtype \
  -DCMAKE_INSTALL_PREFIX=$installdir \
  -DCMAKE_C_COMPILER=$installdir/bin/clang \
  -DCMAKE_CXX_COMPILER=$installdir/bin/clang++ \
  -DCMAKE_Fortran_COMPILER=$installdir/bin/flang \
  -DLLVM_CONFIG=$installdir/bin/llvm-config \
  -DLLVM_TARGETS_TO_BUILD="X86;AArch64" \
  -DLLVM_EXTERNAL_LIT="$llvm_lit" \
  -DFLANG_LLVM_EXTENSIONS=ON \
  ../../flang || exit 1
nice -n 10 make -j48 install || exit 1
nice -n 10 make check-all || exit 1
popd

echo "SUCCESS"

[kiranchandramohan]

Two quick questions.

[kiranchandramohan]

I have fixed this pull request to allow normal Clang builds and regression tests to succeed, except for the two tests mentioned in issue #2. Ideally we should fix those tests too so that we have a clean branch.

I notice that the build instructions you have do not use the FLANG_LLVM_EXTENSIONS flag while building flang. If you use this flag then there will be DIGlobalVariables without the name field. This happens because there is some code (see link below) in lldebug.cpp which removes names of artificial variables.
https://github.com/flang-compiler/flang/blob/d99e82b764a6622e31bec31e61a80072f5c64291/tools/flang2/flang2exe/lldebug.cpp#L529

The simplest testcase to reproduce the issue is,

module IEEE_ARITHMETIC
  use, intrinsic :: iso_c_binding
end module

[bryanpkc]

I notice that the build instructions you have do not use the FLANG_LLVM_EXTENSIONS flag while building flang. If you use this flag then there will be DIGlobalVariables without the name field. This happens because there is some code (see link below) in lldebug.cpp which removes names of artificial variables.

I based my build instructions on the official build instructions which also did not mention FLANG_LLVM_EXTENSIONS. Let me try to look at the DIGlobalVariable problem a bit further.

[bryanpkc]

@kiranchandramohan I believe it is best to combine FLANG_LLVM_EXTENSIONS and USE_CLASSIC_FLANG into one macro, e.g. CLASSIC_FLANG_LLVM_EXTENSIONS, and use this to switch between the two definitions of DIGlobalVariable, along with other Flang-specific features such as the frontend changes and PGMATH. We could then exclude the two failing unit tests when testing Classic Flang. I can update the branch if you agree that this is the right approach.

[peterwaller-arm]

As someone who has been involved in monorepo and driver work elsewhere, this looks broadly good to me. I've reviewed this by studying the diff-of-the-diff vs the release_90 branches on flang-compiler/flang-driver and flang-compiler/llvm.

I think it's possible to simplify the driver, removing the FlangFrontend and instead using the Compiler phase. I've published my preliminary patch, which I will submit for review after this PR has landed, or you're welcome to incorporate those changes here if you feel that is appropriate. I don't mind rebasing as needed for any changes you want to make or include in this patch.

I've highlighted some small issues. I don't think fixing them all needs to hold this PR back.

• I'd like to see the warning fixed, please.
• I think merging the two #defines into one is a good idea and have a personal preference for USE_CLASSIC_FLANG, but I'm sure other names would be fine too.

[peterwaller-arm]

Looks good to me, thanks!

[kiranchandramohan]

LGTM.
Builds and passes make check-all (llvm and flang) with a Release build on a AArch64 machine.

[kiranchandramohan]

This now needs approval from @gklimowicz and @shivaramaarao.

[gklimowicz]

I'm working on this today. It may not be done until Wednesday.

[shivaramaarao]

LGTM. build passed and also the tests passed.

[gklimowicz]

These changes for classic-flang-llvm-project look fine to me. I have run across 2 failures in check-clang on OpenPOWER, though. The first is in f90_correct/fc30. The second is in f90_correct/ls07.

These failures do not occur on LLVM 10 x86_64 or aarch64.

After digging into these failures, they appear to be related to how LLVM 10 on Power is handling negative zeros during the computation of COMPLEX multiplies. The two tests fail when they generate -0.0 for the imaginary part of a COMPLEX number, when the test results are expecting 0.0.

These need to be investigated further. There's a slight chance these are related to the driver changes in the pull request. It seems more likely to be on the LLVM 10 code-gen side.

I see no reason at this time to hold up approval of this pull request, though.

[bryanpkc]

I have pushed flang-compiler/flang#927 to exclude the test failures in f90_correct and created an issue flang-compiler/flang#928 to track their fix.

[bryanpkc]

@kiranchandramohan I don't have merge privilege in this repository. Could you merge this PR, or give me the access please? I think the PR is good to go now that we have a workaround (even if it hasn't been merged) for the test failures.

[gklimowicz]

@bryanpkc I am adding you to the organization, and then will make sure that you have merge privileges.

[kiranchandramohan]

@bryanpkc I hope @gklimowicz has given you commit access and u can merge now. If not then please let me know.

[bryanpkc]

@kiranchandramohan @gklimowicz has added me to the organization but I don't have merge privilege for this repository yet.

[kiranchandramohan]

OK @bryanpkc, i can merge. Is merge (no rebase or squash) the best action here?

[bryanpkc]

Please rebase if necessary, but do not squash. Keeping the historical commits separate make them easier to rebase to future release branches. Thank you.

[kiranchandramohan]

Flang works on llvm-10 now. Thanks @bryanpkc for your help.

[bryanpkc]

Thanks very much @kiranchandramohan.

[bryanpkc]

@peterwaller-arm Could you submit a pull request for your patch (peterwaller-arm/classic-flang-llvm-project@9078d6c4c8b7)?

[banach-space]

@bryanpkc As discussed in the Flang call yesterday (7/10/20), feel free to cherry-pick that patch and submit it on behalf of Peter. We discussed this with @peterwaller-arm offline and decided that that would be the preferred way.

Peter is currently on a different project and I'm actively working on the driver for the LLVM Flang. If you need reviewers, I volunteer :)

[peterwaller-arm]

I've published a rebase on https://github.com/peterwaller-arm/classic-flang-llvm-project/tree/simplify-driver, that's peterwaller-arm@10dacac. It would be great if someone can pick it up and run with it.

It has an issue: currently the classic_flang.f95 test fails, because the driver thinks that flang is capable of emitting assembly. I think what's going on is that ToolSelector::getTool is combining flang+llvm here: https://github.com/peterwaller-arm/classic-flang-llvm-project/blob/10dacac072ded415449e7350c7ba67c2b500f576/clang/lib/Driver/Driver.cpp#L4110-L4113

As far as I know that is the only issue, but it needs testing with flang and I don't have time to dive in to this for a bit.