[PATCH] D154869: [Flang] [FlangRT] Introduce FlangRT project as solution to Flang's runtime LLVM integration

[Sylvestre Ledru via Phabricator via cfe-commits]

sylvestre.ledru added a comment.

In D154869#4652703 , @DanielCChen 
wrote:

> Sorry about the breakage

This is why we have CI ;)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154869/new/

https://reviews.llvm.org/D154869

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154869: [Flang] [FlangRT] Introduce FlangRT project as solution to Flang's runtime LLVM integration

[Daniel Chen via Phabricator via cfe-commits]

DanielCChen added a comment.

Sorry about the breakage and thanks to @DavidSpickett for revert it. 
We will reconsider all the input and bring it back as a PR in the future.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154869/new/

https://reviews.llvm.org/D154869

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154869: [Flang] [FlangRT] Introduce FlangRT project as solution to Flang's runtime LLVM integration

[Sylvestre Ledru via Phabricator via cfe-commits]

sylvestre.ledru added a comment.

Thanks for the revert!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154869/new/

https://reviews.llvm.org/D154869

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154869: [Flang] [FlangRT] Introduce FlangRT project as solution to Flang's runtime LLVM integration

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

I've reverted this 
https://github.com/llvm/llvm-project/commit/ffc67bb3602a6a9a4f886af362e1f2d7c9821570
 as Linaro's flang and clang bots are red all over and we're also in an upgrade 
period for the build server 
(https://discourse.llvm.org/t/llvm-zorg-migration-to-the-buildbot-v3-9/73749) 
so getting a fix into zorg isn't going to happen for a few days at least.

Whoever decided to land this change please look at the remedies suggested by 
@pscoro and submit PRs to address those and eventually reland this. Happy to 
help on review.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154869/new/

https://reviews.llvm.org/D154869

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154869: [Flang] [FlangRT] Introduce FlangRT project as solution to Flang's runtime LLVM integration

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a comment.

This broke a buildbot, as a user reported on discord: 
https://buildkite.com/llvm-project/github-pull-requests/builds/5171#018aec0c-75ba-431a-a5c8-6a5cba51de28


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154869/new/

https://reviews.llvm.org/D154869

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154869: [Flang] [FlangRT] Introduce FlangRT project as solution to Flang's runtime LLVM integration

[Paul Scoropan via Phabricator via cfe-commits]

pscoro added a comment.

In D154869#4652224 , @sylvestre.ledru 
wrote:

> With this patch, it started to fail with:
> It would be nice to have a better error message suggesting what to do
>
>   -- Performing Test HAVE_DECL_STRERROR_S
>   -- Performing Test HAVE_DECL_STRERROR_S - Failed
>   CMake Error at /build/source/flang-rt/unittests/CMakeLists.txt:37 (message):
> Target llvm_gtest not found.
>   
>   
>   -- Configuring incomplete, errors occurred!

Hi, I originally authored this patch but **I am no longer actively involved in 
Flang's development at the moment**, however, I've been getting notifications 
that this patch has been landed yesterday and subsequently, the failing 
buildbots.
I believe all the gtest not found errors originate from a missing build flag, 
as per the documentation page for flang-rt in `flang-rt/docs/GettingStarted.md`:

  # We need to enable GTest if we want to run Flang-rt's testsuites
  -DLLVM_INSTALL_GTEST=On \

I believe I saw that clang buildbots also got inadvertently by this cmake 
change as well.
Wish I had time to investigate this further myself, hopefully @madanial can 
take a closer look. I believe what needs to be done is:

- flang buildbots need their build flags updated to include 
`-DLLVM_INSTALL_GTEST=On`
- The `Target llvm_gtest not found.` should be improved to offer better guidance
- The clang failures should be looked at closer; see if its possible to make a 
change to the cmake to limit the scope of builds needing the 
`-DLLVM_INSTALL_GTEST=On` flag update to just be flang builds. If there is no 
simple way to limit the scope, then updating the build flags for the clang 
builders is another solution
- I noticed at least one flang builder 
(https://lab.llvm.org/buildbot/#/builders/191/builds/24124) is failing build 
with:

  FAILED: bin/external-hello-world 
  : && /usr/local/bin/c++ -fPIC -fno-semantic-interposition 
-fvisibility-inlines-hidden -Werror=date-time 
-Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter 
-Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic 
-Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough 
-Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor 
-Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion 
-Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color 
-ffunction-sections -fdata-sections -Wno-deprecated-copy -Wno-string-conversion 
-Wno-ctad-maybe-unsupported -Wno-unused-command-line-argument 
-Wstring-conversion   -Wcovered-switch-default -Wno-nested-anon-types 
-O3 -DNDEBUG 
-Wl,-rpath-link,/home/tcwg-buildbot/worker/flang-aarch64-rel-assert/build/./lib 
 -Wl,--gc-sections 
tools/flang/examples/ExternalHelloWorld/CMakeFiles/external-hello-world.dir/external-hello.cpp.o
 -o bin/external-hello-world  -Wl,-rpath,"\$ORIGIN/../lib:"  -lpthread  
-lFortranRuntime && :
  /usr/bin/ld: cannot find -lFortranRuntime

This looks to be a usage of the old FortranRuntime configuration that I 
overlooked and so it wasnt updated. This should be linking flang-rt instead of 
FortranRuntime

Hopefully addressing these small issues will get the buildbots green again, 
sorry for the disruptions.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154869/new/

https://reviews.llvm.org/D154869

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154869: [Flang] [FlangRT] Introduce FlangRT project as solution to Flang's runtime LLVM integration

[Sylvestre Ledru via Phabricator via cfe-commits]

sylvestre.ledru added a comment.

With this patch, it started to fail with:
It would be nice to have a better error message suggesting what to do

  -- Performing Test HAVE_DECL_STRERROR_S
  -- Performing Test HAVE_DECL_STRERROR_S - Failed
  CMake Error at /build/source/flang-rt/unittests/CMakeLists.txt:37 (message):
Target llvm_gtest not found.
  
  
  -- Configuring incomplete, errors occurred!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154869/new/

https://reviews.llvm.org/D154869

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154869: [Flang] [FlangRT] Introduce FlangRT project as solution to Flang's runtime LLVM integration

[Mark Danial via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG6403287eff71: [Flang] [FlangRT] Introduce FlangRT project as 
solution to Flangs runtime LLVM… (authored by pscoro, committed by 
madanial).

Changed prior to commit:
  https://reviews.llvm.org/D154869?vs=557468=557508#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154869/new/

https://reviews.llvm.org/D154869

Files:
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  flang-rt/CMakeLists.txt
  flang-rt/docs/GettingStarted.md
  flang-rt/test/CMakeLists.txt
  flang-rt/test/FortranRuntime/no-cpp-dep.c
  flang-rt/test/NonGtestUnit/lit.cfg.py
  flang-rt/test/NonGtestUnit/lit.site.cfg.py.in
  flang-rt/test/Unit/lit.cfg.py
  flang-rt/test/Unit/lit.site.cfg.py.in
  flang-rt/test/lit.cfg.py
  flang-rt/test/lit.site.cfg.py.in
  flang-rt/unittests/CMakeLists.txt
  flang-rt/unittests/FortranEvaluate/CMakeLists.txt
  flang-rt/unittests/FortranEvaluate/ISO-Fortran-binding.cpp
  flang-rt/unittests/FortranEvaluate/reshape.cpp
  flang-rt/unittests/FortranEvaluate/testing.cpp
  flang-rt/unittests/FortranEvaluate/testing.h
  flang-rt/unittests/FortranRuntime/Allocatable.cpp
  flang-rt/unittests/FortranRuntime/ArrayConstructor.cpp
  flang-rt/unittests/FortranRuntime/BufferTest.cpp
  flang-rt/unittests/FortranRuntime/CMakeLists.txt
  flang-rt/unittests/FortranRuntime/CharacterTest.cpp
  flang-rt/unittests/FortranRuntime/CommandTest.cpp
  flang-rt/unittests/FortranRuntime/Complex.cpp
  flang-rt/unittests/FortranRuntime/CrashHandlerFixture.cpp
  flang-rt/unittests/FortranRuntime/CrashHandlerFixture.h
  flang-rt/unittests/FortranRuntime/Derived.cpp
  flang-rt/unittests/FortranRuntime/ExternalIOTest.cpp
  flang-rt/unittests/FortranRuntime/Format.cpp
  flang-rt/unittests/FortranRuntime/Inquiry.cpp
  flang-rt/unittests/FortranRuntime/ListInputTest.cpp
  flang-rt/unittests/FortranRuntime/LogicalFormatTest.cpp
  flang-rt/unittests/FortranRuntime/Matmul.cpp
  flang-rt/unittests/FortranRuntime/MatmulTranspose.cpp
  flang-rt/unittests/FortranRuntime/MiscIntrinsic.cpp
  flang-rt/unittests/FortranRuntime/Namelist.cpp
  flang-rt/unittests/FortranRuntime/Numeric.cpp
  flang-rt/unittests/FortranRuntime/NumericalFormatTest.cpp
  flang-rt/unittests/FortranRuntime/Pointer.cpp
  flang-rt/unittests/FortranRuntime/Ragged.cpp
  flang-rt/unittests/FortranRuntime/Random.cpp
  flang-rt/unittests/FortranRuntime/Reduction.cpp
  flang-rt/unittests/FortranRuntime/RuntimeCrashTest.cpp
  flang-rt/unittests/FortranRuntime/Stop.cpp
  flang-rt/unittests/FortranRuntime/TemporaryStack.cpp
  flang-rt/unittests/FortranRuntime/Time.cpp
  flang-rt/unittests/FortranRuntime/Transformational.cpp
  flang-rt/unittests/FortranRuntime/tools.h
  flang/CMakeLists.txt
  flang/cmake/modules/AddFlang.cmake
  flang/cmake/modules/FlangConfig.cmake.in
  flang/lib/Decimal/CMakeLists.txt
  flang/runtime/CMakeLists.txt
  flang/runtime/sum.cpp
  flang/test/CMakeLists.txt
  flang/test/Driver/linker-flags.f90
  flang/test/lit.cfg.py
  flang/tools/flang-driver/CMakeLists.txt
  flang/unittests/CMakeLists.txt
  flang/unittests/Evaluate/CMakeLists.txt
  flang/unittests/Evaluate/ISO-Fortran-binding.cpp
  flang/unittests/Evaluate/reshape.cpp
  flang/unittests/Optimizer/CMakeLists.txt
  flang/unittests/Runtime/Allocatable.cpp
  flang/unittests/Runtime/ArrayConstructor.cpp
  flang/unittests/Runtime/BufferTest.cpp
  flang/unittests/Runtime/CMakeLists.txt
  flang/unittests/Runtime/CharacterTest.cpp
  flang/unittests/Runtime/CommandTest.cpp
  flang/unittests/Runtime/Complex.cpp
  flang/unittests/Runtime/CrashHandlerFixture.cpp
  flang/unittests/Runtime/CrashHandlerFixture.h
  flang/unittests/Runtime/Derived.cpp
  flang/unittests/Runtime/ExternalIOTest.cpp
  flang/unittests/Runtime/Format.cpp
  flang/unittests/Runtime/Inquiry.cpp
  flang/unittests/Runtime/ListInputTest.cpp
  flang/unittests/Runtime/LogicalFormatTest.cpp
  flang/unittests/Runtime/Matmul.cpp
  flang/unittests/Runtime/MatmulTranspose.cpp
  flang/unittests/Runtime/MiscIntrinsic.cpp
  flang/unittests/Runtime/Namelist.cpp
  flang/unittests/Runtime/Numeric.cpp
  flang/unittests/Runtime/NumericalFormatTest.cpp
  flang/unittests/Runtime/Pointer.cpp
  flang/unittests/Runtime/Ragged.cpp
  flang/unittests/Runtime/Random.cpp
  flang/unittests/Runtime/Reduction.cpp
  flang/unittests/Runtime/RuntimeCrashTest.cpp
  flang/unittests/Runtime/Stop.cpp
  flang/unittests/Runtime/TemporaryStack.cpp
  flang/unittests/Runtime/Time.cpp
  flang/unittests/Runtime/Transformational.cpp
  flang/unittests/Runtime/tools.h
  lld/COFF/MinGW.cpp
  llvm/CMakeLists.txt
  llvm/projects/CMakeLists.txt
  llvm/runtimes/CMakeLists.txt
  runtimes/CMakeLists.txt

Index: runtimes/CMakeLists.txt
===
--- runtimes/CMakeLists.txt
+++ 

[PATCH] D154869: [Flang] [FlangRT] Introduce FlangRT project as solution to Flang's runtime LLVM integration

[Daniel Chen via Phabricator via cfe-commits]

DanielCChen accepted this revision.
DanielCChen added a comment.

@all
It seems all the comments are addressed or explained. If there is no objection 
from the reviewers, we would like to land this patch in Phabricator before the 
deadline. 
I will approve it now, but @madanial please wait until the EOD to see if other 
reviewers have further comments.

We will address any further suggestion of change as separate PRs.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154869/new/

https://reviews.llvm.org/D154869

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154869: [Flang] [FlangRT] Introduce FlangRT project as solution to Flang's runtime LLVM integration

[Mark Danial via Phabricator via cfe-commits]

madanial added a comment.

In D154869#4628847 , @efriedma wrote:

> Maybe split the changes to reformat the unittests into a separate patch?
>
> Otherwise, I'm happy with the current state of this patch, but probably 
> someone more active in flang should approve.

I believe the changes are very trival to the unittests, would we be able to 
keep them as part of this patch and avoid moving it to github


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154869/new/

https://reviews.llvm.org/D154869

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154869: [Flang] [FlangRT] Introduce FlangRT project as solution to Flang's runtime LLVM integration

[Mark Danial via Phabricator via cfe-commits]

madanial added inline comments.



Comment at: runtimes/CMakeLists.txt:20
 
+set(RUNTIMES_SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR})
+

ldionne wrote:
> This shouldn't be necessary, CMake already creates a `Project_SOURCE_DIR` 
> variable for each declared `project()` IIRC.
Updated thanks for the review


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154869/new/

https://reviews.llvm.org/D154869

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154869: [Flang] [FlangRT] Introduce FlangRT project as solution to Flang's runtime LLVM integration

[H. Vetinari via Phabricator via cfe-commits]

h-vetinari added a comment.

Any update on this patchset? With the migration away from phabricator, it'd be 
good to get this in soonish (otherwise it'll need to be moved to github).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154869/new/

https://reviews.llvm.org/D154869

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154869: [Flang] [FlangRT] Introduce FlangRT project as solution to Flang's runtime LLVM integration

[Louis Dionne via Phabricator via cfe-commits]

ldionne added inline comments.



Comment at: runtimes/CMakeLists.txt:20
 
+set(RUNTIMES_SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR})
+

This shouldn't be necessary, CMake already creates a `Project_SOURCE_DIR` 
variable for each declared `project()` IIRC.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154869/new/

https://reviews.llvm.org/D154869

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154869: [Flang] [FlangRT] Introduce FlangRT project as solution to Flang's runtime LLVM integration

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

Maybe split the changes to reformat the unittests into a separate patch?

Otherwise, I'm happy with the current state of this patch, but probably someone 
more active in flang should approve.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154869/new/

https://reviews.llvm.org/D154869

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154869: [Flang] [FlangRT] Introduce FlangRT project as solution to Flang's runtime LLVM integration

[Mark Danial via Phabricator via cfe-commits]

madanial added a comment.

Pinging for review. I will be taking over this patch from Paul, but Paul will 
still be around to answer any questions.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154869/new/

https://reviews.llvm.org/D154869

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154869: [Flang] [FlangRT] Introduce FlangRT project as solution to Flang's runtime LLVM integration

[Paul Scoropan via Phabricator via cfe-commits]

pscoro added inline comments.



Comment at: flang-rt/unittests/FortranEvaluate/testing.h:1
+#ifndef FORTRAN_EVALUATE_TESTING_H_
+#define FORTRAN_EVALUATE_TESTING_H_

klausler wrote:
> Why was this file (and testing.cpp) added instead of being moved, like other 
> files were?
> 
> Why are the unit tests for the Evaluate library being moved into flang-rt?  
> The Evaluate library is only used by the compiler for constant folding and 
> expression processing; it is not part of the runtime.
There are 2 unit tests (reshape and ISO-Fortran-Binding) that include runtime 
headers. Neither of these tests included headers from any other Flang libraries 
so I chose to move them under flang-rt. I preserved the testsuite name 
FortranEvaluate because thats where they came from. I am not too confident in 
this decision. Let me know if there is an alternative you prefer.

These testing files were copied because they are needed for other tests that 
remained in flang/unittests/FortranEvaluate and for these 2 unit tests that I 
moved to flang-rt


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154869/new/

https://reviews.llvm.org/D154869

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154869: [Flang] [FlangRT] Introduce FlangRT project as solution to Flang's runtime LLVM integration

[Peter Klausler via Phabricator via cfe-commits]

klausler added inline comments.



Comment at: flang-rt/unittests/FortranEvaluate/testing.h:1
+#ifndef FORTRAN_EVALUATE_TESTING_H_
+#define FORTRAN_EVALUATE_TESTING_H_

Why was this file (and testing.cpp) added instead of being moved, like other 
files were?

Why are the unit tests for the Evaluate library being moved into flang-rt?  The 
Evaluate library is only used by the compiler for constant folding and 
expression processing; it is not part of the runtime.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154869/new/

https://reviews.llvm.org/D154869

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154869: [Flang] [FlangRT] Introduce FlangRT project as solution to Flang's runtime LLVM integration

[Paul Scoropan via Phabricator via cfe-commits]

pscoro added a comment.

Ping

I will not be able to help out with flang (for a while) after the end of August 
so the sooner I can get this reviewed the smoother the landing process will be 
for myself and the community. So I would appreciate if I could get reviewers 
looking at this as soon as possible. Thanks


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154869/new/

https://reviews.llvm.org/D154869

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits