Skip to content

[NFC] Adding missing flang tests for LTO bugfix #80417

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

Conversation

paschalis-mpeis
Copy link
Member

Pull request #78749 introduced a fix for passing vector library flags from clang frontend to opt when LTO is used.

The fix also covered flang, as it's handled in CommonArgs.cpp, but no testing was added for flang.

Pull request #78749 introduced a fix for passing vector library flags
from clang frontend to opt when LTO is used.

The fix also covered flang, as it's handled in CommonArgs.cpp, but no
testing was added for flang.
@llvmbot llvmbot added flang:driver flang Flang issues not falling into any other category labels Feb 2, 2024
@llvmbot
Copy link
Member

llvmbot commented Feb 2, 2024

@llvm/pr-subscribers-flang-driver

Author: Paschalis Mpeis (paschalis-mpeis)

Changes

Pull request #78749 introduced a fix for passing vector library flags from clang frontend to opt when LTO is used.

The fix also covered flang, as it's handled in CommonArgs.cpp, but no testing was added for flang.


Full diff: https://github.com/llvm/llvm-project/pull/80417.diff

1 Files Affected:

  • (modified) flang/test/Driver/fveclib.f90 (+17)
diff --git a/flang/test/Driver/fveclib.f90 b/flang/test/Driver/fveclib.f90
index 14c59b0616f82..76071c2255a47 100644
--- a/flang/test/Driver/fveclib.f90
+++ b/flang/test/Driver/fveclib.f90
@@ -30,3 +30,20 @@
 
 ! TODO: if we add support for -nostdlib or -nodefaultlibs we need to test that
 ! these prevent "-framework Accelerate" being added on Darwin
+
+! Verify that the correct vector library is passed to LTO flags.
+
+! RUN: %flang -### --target=x86_64-unknown-linux-gnu -fveclib=LIBMVEC -flto %s 2>&1 | FileCheck -check-prefix CHECK-LTO-LIBMVEC %s
+! CHECK-LTO-LIBMVEC: "-plugin-opt=-vector-library=LIBMVEC-X86"
+
+! RUN: %flang -### --target=powerpc64-unknown-linux-gnu -fveclib=MASSV -flto %s 2>&1 | FileCheck -check-prefix CHECK-LTO-MASSV %s
+! CHECK-LTO-MASSV: "-plugin-opt=-vector-library=MASSV"
+
+! RUN: %flang -### --target=x86_64-unknown-linux-gnu -fveclib=SVML -flto %s 2>&1 | FileCheck -check-prefix CHECK-LTO-SVML %s
+! CHECK-LTO-SVML: "-plugin-opt=-vector-library=SVML"
+
+! RUN: %flang -### --target=aarch64-linux-gnu -fveclib=SLEEF -flto %s 2>&1 | FileCheck -check-prefix CHECK-LTO-SLEEF %s
+! CHECK-LTO-SLEEF: "-plugin-opt=-vector-library=sleefgnuabi"
+
+! RUN: %flang -### --target=aarch64-linux-gnu -fveclib=ArmPL -flto %s 2>&1 | FileCheck -check-prefix CHECK-LTO-ARMPL %s
+! CHECK-LTO-ARMPL: "-plugin-opt=-vector-library=ArmPL"

Copy link
Contributor

@tblah tblah left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for adding the tests

@paschalis-mpeis
Copy link
Member Author

Thanks Tom.

For the rest of the reviewers, the tests added are mirroring these clang tests

@banach-space
Copy link
Contributor

Thanks for this patch!

The fix also covered flang, as it's handled in CommonArgs.cpp, but no testing was added for flang.

From your summary, this particular logic in CommonArgs.cpp is already tested in Clang. IMHO, we should avoid duplicating tests like this. Sometimes that's tricky to avoid, especially when enabling new options in Flang. However, in this case, it seems quite apparent that testing in Flang is not going to increase the test coverage. I might be missing something though.

@paschalis-mpeis
Copy link
Member Author

@banach-space thanks for your comment.

As this is not aligned with on-going and future direction of flags in clang/flang fron-ends, I've decided to not land this patch.

@paschalis-mpeis paschalis-mpeis deleted the users/paschalis-mpeis/missing-tests-veclib-lto-flags branch February 5, 2024 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flang:driver flang Flang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants