-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[flang][acc] Ensure fir.class is handled in type categorization #146174
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
Open
razvanlupusoru
wants to merge
4
commits into
llvm:main
Choose a base branch
from
razvanlupusoru:firclassacc
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+86
−6
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
! RUN: bbc -fopenacc -emit-hlfir %s -o - | fir-opt -pass-pipeline='builtin.module(test-fir-openacc-interfaces)' --mlir-disable-threading 2>&1 | FileCheck %s | ||
|
||
module mm | ||
type, public :: polyty | ||
real :: field | ||
end type | ||
contains | ||
subroutine init(this) | ||
class(polyty), intent(inout) :: this | ||
!$acc enter data copyin(this, this%field) | ||
Comment on lines
+9
to
+10
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have you check if it works as well with an unlimited polymorphic dummy?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It did not - and now I fixed it as I described below. |
||
end subroutine | ||
subroutine init_assumed_type(var) | ||
type(*), intent(inout) :: var | ||
!$acc enter data copyin(var) | ||
end subroutine | ||
subroutine init_unlimited(this) | ||
class(*), intent(inout) :: this | ||
!$acc enter data copyin(this) | ||
select type(this) | ||
type is(real) | ||
!$acc enter data copyin(this) | ||
class is(polyty) | ||
!$acc enter data copyin(this, this%field) | ||
end select | ||
end subroutine | ||
end module | ||
|
||
! CHECK: Visiting: {{.*}} acc.copyin {{.*}} {name = "this", structured = false} | ||
! CHECK: Mappable: !fir.class<!fir.type<_QMmmTpolyty{field:f32}>> | ||
! CHECK: Type category: composite | ||
! CHECK: Visiting: {{.*}} acc.copyin {{.*}} {name = "this%field", structured = false} | ||
! CHECK: Pointer-like: !fir.ref<f32> | ||
! CHECK: Type category: composite | ||
|
||
! For unlimited polymorphic entities and assumed types - they effectively have | ||
! no declared type. Thus the type categorizer cannot categorize it. | ||
! CHECK: Visiting: {{.*}} = acc.copyin {{.*}} {name = "var", structured = false} | ||
! CHECK: Pointer-like: !fir.ref<none> | ||
! CHECK: Type category: uncategorized | ||
! CHECK: Visiting: {{.*}} = acc.copyin {{.*}} {name = "this", structured = false} | ||
! CHECK: Mappable: !fir.class<none> | ||
! CHECK: Type category: uncategorized | ||
|
||
! TODO: After using select type - the appropriate type category should be | ||
! possible. Add the rest of the test once OpenACC lowering correctly handles | ||
! unlimited polymorhic. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test should probably go in
flang/test/Lower/OpenACC
or starts from mlir directly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I just saw that you added another test like that in this folder as well. No strong opinion on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer the test this way - changing FIR directly, especially for derived types - is not quite the nicest experience due to the verbosity. I hope you can be OK with this!