- User Since
- Apr 21 2016, 3:27 AM (322 w, 6 d)
Missed a spot ;) Otherwise LGTM
No change, trying to convince the pre-merge to apply the patch...
Added more comments.
Hopefully the pre-merge CI will actually run this time...
Mon, Jun 27
Right, not sure why it doesn't find those files, I'll try a few things...
Try to fix CI patch application - take 2.
Try to fix CI patch application.
Fri, Jun 24
Thu, Jun 23
Not mine either :D
Relax the tolerance only for FM905 and FM907 (which are already special-cased in D128260).
Right, disable :)
Updated the patch to special-case FM905 and FM907. I think it doesn't hurt to leave a bit of list-directed output in the test suite just to make sure it doesn't randomly explode.
One thing that we could do is to only enable whitespace for the 2 tests that use list-directed output. That's going to require a bit more effort in the CMakeLists.txt, but it would let us be strict about whitespace where specific formatting is used, and more relaxed where it isn't.
I just realized I haven't pestered you enough about testing :) Can you add a test that -O4 indeed warns and uses -O3? Also, the summary says this should work in both the compilation and the frontend driver, but you're only testing with %flang_fc1.
Wed, Jun 22
Thanks for the explanation! I have updated the patch description with all the information. fpcmp does treat everything as double precision.
Tue, Jun 21
Yep, LGTM, thanks for the tests!
(Disclaimer: I don't consider myself a Clang developer, so maybe wait a day or 2 before committing in case someone else has comments)
Seems fine, thanks.
Mon, Jun 20
Fri, Jun 17
I'm going to go ahead and commit this on Monday if nobody raises any objections until then. Thanks again to everyone for all the help & feedback!
Thu, Jun 16
Wed, Jun 15
Tue, Jun 14
- Use --implicit-not-check <3
Clarified test checks.
Mon, Jun 13
- Stop using --ld-path, since it seems to be ignored on Windows. Instead, just match any path that ends in ld (should work with ld, lld, gold). This isn't very robust, but I don't have any better ideas (other than actually support --ld-path everywhere, not sure how much work that would be).
I'm guessing the Windows precommit is failing because --ld-path is ignored on Windows, even if we use a Linux target? I have a fix that works on my Windows machine, coming right up.
Missed a spot (removing the namespace in MinGW.cpp)
- Switch to the new style of testing (using triples rather than separate files)
- Fix oversight in Darwin toolchain file
I had the same idea about switching the tests to using target triples instead of having separate files for it, but initially I had some issues getting that to work properly. When specifying a triple, we need to provide an architecture. Leaving the triple as unkown-linux-gnu or just linux-gnu gives us an error along the lines of flang-new: error: unknown target triple 'unknown-unknown-linux-gnu', please use -triple or -arch. OTOH, hardcoding an architecture like x86 or aarch64 fails if we're not building that specific backend. We could do that and make the test REQUIRE the architecture that we're hardcoding, but this isn't really an architecture-specific test. So what I've finally done instead is to check for flang supported architectures and add a lit substitution for the first one that we find (be it aarch64, powerpc or x86) and use that in the test. We'll still get an error if someone tries to build the test without enabling any of these targets, but I think that's a good thing, since then people can decide either to add their architecture to lit or just, you know, not build flang on platforms where it isn't supported :) Patch incoming.
Fri, Jun 10
Moved the check for -flang-experimental-exec into addFortranRuntimeLibraryPath, so it affects all the toolchains. @awarzynski does this look like a good idea?
Thanks for submitting this! I think the NOMINMAX trick should fix the issues I was seeing on Windows on Arm, but I still need to test to be sure (it's slow...). Do you have commit access or should I commit on your behalf?
One small Phabricator nitpick: it's nice to upload patches with context (you don't have to do that for this patch, I was just mentioning it for the future).
Wed, Jun 8
- Fixed test
- Unconditionally added the subsystem arg
- Incorporated the MinGW toolchain changes (Thanks again @mmuetzel, I'm adding you as co-author)
This LGTM, but please wait another day or two before committing in case anyone else still has comments.
Thanks, this LGTM but I'll let @clementval have the final word :)
Fri, Jun 3
@mmuetzel Thanks for looking into this! I think since you're passing -DCLANG_DEFAULT_RTLIB=compiler-rt, you might indeed need to build compiler-rt (or at least the builtins part of it).
This looks good in general, but it's missing a couple of tests:
- a test with a file with a .mlir extension
- tests for the -x option
- Update for MinGW.
- Add /subsystem:console to help link.exe understand what's going on.
Thu, Jun 2
Wed, Jun 1
Updated comment in linker-flags.f90 test
Tue, May 31
Updated test to check for link.exe instead of lld-link.exe. This doesn't look great but it works for both lld-link.exe and the default link.exe. I tried to use --ld-path=various/paths instead, but it seems to be ignored on Windows (I even get a warning about "argument unused during compilation"). If anyone has any ideas about how to make the test more robust, please let me know.
May 30 2022
May 27 2022
May 25 2022
This is an alternative to https://reviews.llvm.org/D126279
I personally prefer the other patch, for reasons which I've explained there.
If we go with this approach, I'll also send a patch to the docs to document the need for setting this environment variable.
I can do that, but I don't like that solution. My understanding is that these warnings are platform-dependent, and also the 'Fortran STOP' seems to be printed by default by flang. This means pretty much anyone running the test-suite with flang will have to set this environment variable, or else face a ton of failures. Why make the test-suite harder to run? I think one of the goals of the test-suite is to be independent from both the hardware and the compiler, and all the better if it's possible to achieve that without compiler-specific hacks. In this case, setting the STOP statement to QUIET makes sense to me because its output is not strictly defined by the standard and will vary with both hardware and compiler. The test-suite is not a good place to test warnings, error messages or other fuzzy output - that should go in flang's own tests.
May 24 2022
May 19 2022
Seems legit, thank you!