Skip to content

[flang] Fix typos PPC intrinsics tests (NFC) #92943

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

Merged
merged 4 commits into from
May 28, 2024
Merged

Conversation

kkwli
Copy link
Collaborator

@kkwli kkwli commented May 21, 2024

@kkwli kkwli requested a review from DanielCChen May 21, 2024 17:59
@kkwli kkwli self-assigned this May 21, 2024
@llvmbot llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir labels May 21, 2024
@llvmbot
Copy link
Member

llvmbot commented May 21, 2024

@llvm/pr-subscribers-flang-fir-hlfir

Author: Kelvin Li (kkwli)

Changes

See #92387 (comment)


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

2 Files Affected:

  • (modified) flang/test/Lower/PowerPC/ppc-vec-load.f90 (+1)
  • (modified) flang/test/Lower/PowerPC/ppc-vec-shift-be-le.f90 (+1-1)
diff --git a/flang/test/Lower/PowerPC/ppc-vec-load.f90 b/flang/test/Lower/PowerPC/ppc-vec-load.f90
index 4d51512df0f7b..61f18a79cf901 100644
--- a/flang/test/Lower/PowerPC/ppc-vec-load.f90
+++ b/flang/test/Lower/PowerPC/ppc-vec-load.f90
@@ -1,4 +1,5 @@
 ! RUN: %flang_fc1 -flang-experimental-hlfir -triple powerpc64le-unknown-unknown -emit-llvm %s -o - | FileCheck --check-prefixes="LLVMIR","LLVMIR-LE" %s
+! RUN: %flang_fc1 -triple powerpc64le-unknown-unknown -target-cpu pwr9 -emit-llvm %s -o - | FileCheck --check-prefixes="LLVMIR","LLVMIR_P9" %s
 ! RUN: %flang_fc1 -flang-experimental-hlfir -triple powerpc64-unknown-unknown -emit-llvm %s -o - | FileCheck --check-prefixes="LLVMIR","LLVMIR-BE" %s
 ! REQUIRES: target=powerpc{{.*}}
 
diff --git a/flang/test/Lower/PowerPC/ppc-vec-shift-be-le.f90 b/flang/test/Lower/PowerPC/ppc-vec-shift-be-le.f90
index bd83f28b4eeb5..61dc9d08b1a02 100644
--- a/flang/test/Lower/PowerPC/ppc-vec-shift-be-le.f90
+++ b/flang/test/Lower/PowerPC/ppc-vec-shift-be-le.f90
@@ -1,4 +1,4 @@
-! RUN: %flang_fc1 -flang-experimental-hlfir -emit-llvm %s -triple ppc64le-unknown-linux -o - | FileCheck --check-prefixes="CHECK" %s
+! RUN: %flang_fc1 -flang-experimental-hlfir -emit-llvm %s -triple ppc64le-unknown-linux -o - | FileCheck --check-prefixes="LLVMIR" %s
 !
 ! RUN: %flang_fc1 -flang-experimental-hlfir -emit-llvm %s -triple ppc64-unknown-aix -o - | FileCheck --check-prefixes="BE-LLVMIR" %s
 ! REQUIRES: target=powerpc{{.*}}

Copy link
Contributor

@DanielCChen DanielCChen left a comment

Choose a reason for hiding this comment

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

LGTM.

! RUN: %flang_fc1 -flang-experimental-hlfir -emit-llvm %s -triple ppc64le-unknown-linux -o - | FileCheck --check-prefixes="CHECK" %s
! RUN: %flang_fc1 -flang-experimental-hlfir -emit-llvm %s -triple ppc64le-unknown-linux -o - | FileCheck --check-prefixes="LLVMIR" %s
Copy link
Contributor

@klensy klensy May 21, 2024

Choose a reason for hiding this comment

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

Isn't this makes CHECK prefixes unused? For example, on 10 line.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, you're right. I will update it. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

While currently CHECK-LABEL is allowed check prefix, this soon can be not (#92735). Plus this converts https://llvm.org/docs/CommandGuide/FileCheck.html#the-check-label-directive to simple prefix(or not? Linked pr should fix that).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the heads up. I think I'd better use another prefix.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm losing confidence in my understanding how different prefixes works between them: in case of

FOO-LABEL: xxx
BAR: yyy
...
FOO2-LABEL: zzz

will BAR me matched between FOO and FOO2 or not? idk.

Anyway, in 76100a9 you defined prefixes ending with -LABEL, but they should be LLVM, i guess. So you need to change only definitions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Anyway, in 76100a9 you defined prefixes ending with -LABEL, but they should be LLVM, i guess. So you need to change only definitions

Sorry, I don't quite understand. Did you mean if I specify LLVM in --check-prefixes, LLVM-LABEL will be effective?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this should start working as directive LABEL, if i understand correct.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks. I push a commit to fix that.

@kkwli
Copy link
Collaborator Author

kkwli commented May 24, 2024

@klensy Thanks for the review. Do you have any further comment? If not, I will merge it.

@klensy
Copy link
Contributor

klensy commented May 28, 2024

@klensy Thanks for the review. Do you have any further comment? If not, I will merge it.

Ok, should be good now.

@kkwli
Copy link
Collaborator Author

kkwli commented May 28, 2024

Ok, should be good now.

Thanks for the review.

@kkwli kkwli merged commit 1da52ca into llvm:main May 28, 2024
7 checks passed
@kkwli kkwli deleted the fix-ppc-lit branch May 28, 2024 12:59
vg0204 pushed a commit to vg0204/llvm-project that referenced this pull request May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flang:fir-hlfir flang Flang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants