Skip to content

Conversation

@mandlebug
Copy link
Member

Test was running both -mcpu=pwrN and -mcpu=powerN compile steps for power N = 8, 9 and 10. Reduce to one run step for each form using power8 for one instance and power 10 for the other.

Test was running both -mcpu=pwrN and -mcpu=powerN compile steps for
power N = 8, 9 and 10. Reduce to one run step for each form using power8
for one instance and power 10 for the other.
@mandlebug mandlebug requested review from RolandF77 and amy-kwan May 12, 2025 19:22
@mandlebug mandlebug self-assigned this May 12, 2025
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels May 12, 2025
@llvmbot
Copy link
Member

llvmbot commented May 12, 2025

@llvm/pr-subscribers-clang-driver

Author: Sean Fertile (mandlebug)

Changes

Test was running both -mcpu=pwrN and -mcpu=powerN compile steps for power N = 8, 9 and 10. Reduce to one run step for each form using power8 for one instance and power 10 for the other.


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

1 Files Affected:

  • (modified) clang/test/Driver/ppc-mrop-protection-support-check.c (-10)
diff --git a/clang/test/Driver/ppc-mrop-protection-support-check.c b/clang/test/Driver/ppc-mrop-protection-support-check.c
index 50eaef3ed770b..541dc2bf99c3f 100644
--- a/clang/test/Driver/ppc-mrop-protection-support-check.c
+++ b/clang/test/Driver/ppc-mrop-protection-support-check.c
@@ -1,20 +1,10 @@
 // RUN: not %clang -target powerpc64le-unknown-linux-gnu -fsyntax-only \
 // RUN:   -mcpu=pwr10 -mrop-protect %s 2>&1 | FileCheck %s --check-prefix=HASROP
 // RUN: not %clang -target powerpc64le-unknown-linux-gnu -fsyntax-only \
-// RUN:   -mcpu=power10 -mrop-protect %s 2>&1 | FileCheck %s --check-prefix=HASROP
-// RUN: not %clang -target powerpc64le-unknown-linux-gnu -fsyntax-only \
-// RUN:   -mcpu=pwr9 -mrop-protect %s 2>&1 | FileCheck %s --check-prefix=HASROP
-// RUN: not %clang -target powerpc64le-unknown-linux-gnu -fsyntax-only \
-// RUN:   -mcpu=power9 -mrop-protect %s 2>&1 | FileCheck %s --check-prefix=HASROP
-// RUN: not %clang -target powerpc64le-unknown-linux-gnu -fsyntax-only \
-// RUN:   -mcpu=pwr8 -mrop-protect %s 2>&1 | FileCheck %s --check-prefix=HASROP
-// RUN: not %clang -target powerpc64le-unknown-linux-gnu -fsyntax-only \
 // RUN:   -mcpu=power8 -mrop-protect %s 2>&1 | FileCheck %s --check-prefix=HASROP
 
 // RUN: not %clang -target powerpc64le-unknown-linux-gnu -fsyntax-only \
 // RUN:   -mcpu=pwr7 -mrop-protect %s 2>&1 | FileCheck %s --check-prefix=NOROP
-// RUN: not %clang -target powerpc64le-unknown-linux-gnu -fsyntax-only \
-// RUN:   -mcpu=power7 -mrop-protect %s 2>&1 | FileCheck %s --check-prefix=NOROP
 
 #ifdef __ROP_PROTECT__
 static_assert(false, "ROP Protect enabled");

@llvmbot
Copy link
Member

llvmbot commented May 12, 2025

@llvm/pr-subscribers-clang

Author: Sean Fertile (mandlebug)

Changes

Test was running both -mcpu=pwrN and -mcpu=powerN compile steps for power N = 8, 9 and 10. Reduce to one run step for each form using power8 for one instance and power 10 for the other.


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

1 Files Affected:

  • (modified) clang/test/Driver/ppc-mrop-protection-support-check.c (-10)
diff --git a/clang/test/Driver/ppc-mrop-protection-support-check.c b/clang/test/Driver/ppc-mrop-protection-support-check.c
index 50eaef3ed770b..541dc2bf99c3f 100644
--- a/clang/test/Driver/ppc-mrop-protection-support-check.c
+++ b/clang/test/Driver/ppc-mrop-protection-support-check.c
@@ -1,20 +1,10 @@
 // RUN: not %clang -target powerpc64le-unknown-linux-gnu -fsyntax-only \
 // RUN:   -mcpu=pwr10 -mrop-protect %s 2>&1 | FileCheck %s --check-prefix=HASROP
 // RUN: not %clang -target powerpc64le-unknown-linux-gnu -fsyntax-only \
-// RUN:   -mcpu=power10 -mrop-protect %s 2>&1 | FileCheck %s --check-prefix=HASROP
-// RUN: not %clang -target powerpc64le-unknown-linux-gnu -fsyntax-only \
-// RUN:   -mcpu=pwr9 -mrop-protect %s 2>&1 | FileCheck %s --check-prefix=HASROP
-// RUN: not %clang -target powerpc64le-unknown-linux-gnu -fsyntax-only \
-// RUN:   -mcpu=power9 -mrop-protect %s 2>&1 | FileCheck %s --check-prefix=HASROP
-// RUN: not %clang -target powerpc64le-unknown-linux-gnu -fsyntax-only \
-// RUN:   -mcpu=pwr8 -mrop-protect %s 2>&1 | FileCheck %s --check-prefix=HASROP
-// RUN: not %clang -target powerpc64le-unknown-linux-gnu -fsyntax-only \
 // RUN:   -mcpu=power8 -mrop-protect %s 2>&1 | FileCheck %s --check-prefix=HASROP
 
 // RUN: not %clang -target powerpc64le-unknown-linux-gnu -fsyntax-only \
 // RUN:   -mcpu=pwr7 -mrop-protect %s 2>&1 | FileCheck %s --check-prefix=NOROP
-// RUN: not %clang -target powerpc64le-unknown-linux-gnu -fsyntax-only \
-// RUN:   -mcpu=power7 -mrop-protect %s 2>&1 | FileCheck %s --check-prefix=NOROP
 
 #ifdef __ROP_PROTECT__
 static_assert(false, "ROP Protect enabled");

@mandlebug mandlebug requested a review from lei137 May 12, 2025 20:18
@mandlebug mandlebug changed the title Reduce number of run steps in ppc rop-protect test. [NFC] Reduce number of run steps in ppc rop-protect test. May 13, 2025
// RUN: not %clang -target powerpc64le-unknown-linux-gnu -fsyntax-only \
// RUN: -mcpu=pwr9 -mrop-protect %s 2>&1 | FileCheck %s --check-prefix=HASROP
// RUN: not %clang -target powerpc64le-unknown-linux-gnu -fsyntax-only \
// RUN: -mcpu=power9 -mrop-protect %s 2>&1 | FileCheck %s --check-prefix=HASROP
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we removing P9 as it should be covered by both P8/P10? Or did we also mean to keep a P9 line?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. We have one test for P8 which is the first CPU version supported, and one test for P10 which is a later CPU that also supports it. Adding a P9 doesn't really add any coverage IMO.

// RUN: not %clang -target powerpc64le-unknown-linux-gnu -fsyntax-only \
// RUN: -mcpu=power9 -mrop-protect %s 2>&1 | FileCheck %s --check-prefix=HASROP
// RUN: not %clang -target powerpc64le-unknown-linux-gnu -fsyntax-only \
// RUN: -mcpu=pwr8 -mrop-protect %s 2>&1 | FileCheck %s --check-prefix=HASROP
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't feel super strongly either way, but should we keep the pwr version for consistency?

Copy link
Member Author

Choose a reason for hiding this comment

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

The P10 run step uses the pwr format. I think one run step for each mcpu format is adequate. Realistically we should have other coverage that ensures that the 2 formats sets up the same environment and this test could use just one format either way, but since we have a run step for a target greater then 8 anyway I thought to test one of each.

Copy link
Collaborator

@RolandF77 RolandF77 left a comment

Choose a reason for hiding this comment

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

LGTM

@mandlebug mandlebug merged commit 0afa62c into llvm:main May 14, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang Clang issues not falling into any other category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants