[flang][acc] Ensure fir.class is handled in type categorization #146174
+86
−6
Conversation
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
fir.class is treated similarly as fir.box - but it has one key distinction which is that it doesn't hold an element type. Thus the categorization logic was mishandling this case for this reason (and also the fact that it assumed that a base object is always a fir.ref). This PR improves this handling and adds appropriate test exercising both a class and a class field to ensure categorization works.
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-fir-hlfir Author: Razvan Lupusoru (razvanlupusoru) Changesfir.class is treated similarly as fir.box - but it has one key distinction which is that it doesn't hold an element type. Thus the categorization logic was mishandling this case for this reason (and also the fact that it assumed that a base object is always a fir.ref). This PR improves this handling and adds appropriate test exercising both a class and a class field to ensure categorization works. Full diff: https://github.com/llvm/llvm-project/pull/146174.diff 3 Files Affected:
diff --git a/flang/lib/Optimizer/OpenACC/FIROpenACCTypeInterfaces.cpp b/flang/lib/Optimizer/OpenACC/FIROpenACCTypeInterfaces.cpp
index 673d7e86c7ba0..2702f7e8c185e 100644
--- a/flang/lib/Optimizer/OpenACC/FIROpenACCTypeInterfaces.cpp
+++ b/flang/lib/Optimizer/OpenACC/FIROpenACCTypeInterfaces.cpp
@@ -320,8 +320,13 @@ template <>
mlir::acc::VariableTypeCategory
OpenACCMappableModel<fir::BaseBoxType>::getTypeCategory(mlir::Type type,
mlir::Value var) const {
+ // Class-type does not behave like a normal box because it does not hold an
+ // element type. Thus special handle it here.
+ if (mlir::isa<fir::ClassType>(type))
+ return mlir::acc::VariableTypeCategory::composite;
mlir::Type eleTy = fir::dyn_cast_ptrOrBoxEleTy(type);
+ assert(eleTy && "expect to be able to unwrap the element type");
// If the type enclosed by the box is a mappable type, then have it
// provide the type category.
@@ -346,7 +351,7 @@ OpenACCMappableModel<fir::BaseBoxType>::getTypeCategory(mlir::Type type,
return mlir::acc::VariableTypeCategory::nonscalar;
}
-static mlir::TypedValue<mlir::acc::PointerLikeType>
+static mlir::Value
getBaseRef(mlir::TypedValue<mlir::acc::PointerLikeType> varPtr) {
// If there is no defining op - the unwrapped reference is the base one.
mlir::Operation *op = varPtr.getDefiningOp();
@@ -372,7 +377,7 @@ getBaseRef(mlir::TypedValue<mlir::acc::PointerLikeType> varPtr) {
})
.Default([&](mlir::Operation *) { return varPtr; });
- return mlir::cast<mlir::TypedValue<mlir::acc::PointerLikeType>>(baseRef);
+ return baseRef;
}
static mlir::acc::VariableTypeCategory
@@ -384,10 +389,17 @@ categorizePointee(mlir::Type pointer,
// value would both be represented as !fir.ref<f32>. We do not want to treat
// such a reference as a scalar. Thus unwrap interior pointer calculations.
auto baseRef = getBaseRef(varPtr);
- mlir::Type eleTy = baseRef.getType().getElementType();
- if (auto mappableTy = mlir::dyn_cast<mlir::acc::MappableType>(eleTy))
- return mappableTy.getTypeCategory(varPtr);
+ if (auto mappableTy =
+ mlir::dyn_cast<mlir::acc::MappableType>(baseRef.getType()))
+ return mappableTy.getTypeCategory(baseRef);
+
+ // It must be a pointer-like type since it is not a MappableType.
+ auto ptrLikeTy = mlir::cast<mlir::acc::PointerLikeType>(baseRef.getType());
+ mlir::Type eleTy = ptrLikeTy.getElementType();
+
+ if (auto mappableEleTy = mlir::dyn_cast<mlir::acc::MappableType>(eleTy))
+ return mappableEleTy.getTypeCategory(varPtr);
if (isScalarLike(eleTy))
return mlir::acc::VariableTypeCategory::scalar;
diff --git a/flang/test/Fir/OpenACC/openacc-type-categories-class.f90 b/flang/test/Fir/OpenACC/openacc-type-categories-class.f90
new file mode 100644
index 0000000000000..0a38ab96a0315
--- /dev/null
+++ b/flang/test/Fir/OpenACC/openacc-type-categories-class.f90
@@ -0,0 +1,18 @@
+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)
+ end subroutine
+end module
+
+! RUN: bbc -fopenacc -emit-hlfir %s -o - | fir-opt -pass-pipeline='builtin.module(test-fir-openacc-interfaces)' --mlir-disable-threading 2>&1 | FileCheck %s
+! 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
diff --git a/flang/test/lib/OpenACC/TestOpenACCInterfaces.cpp b/flang/test/lib/OpenACC/TestOpenACCInterfaces.cpp
index 90aabd7d40d44..11567d1c0c6a3 100644
--- a/flang/test/lib/OpenACC/TestOpenACCInterfaces.cpp
+++ b/flang/test/lib/OpenACC/TestOpenACCInterfaces.cpp
@@ -6,12 +6,16 @@
//
//===----------------------------------------------------------------------===//
+#include "mlir/Dialect/Arith/IR/Arith.h"
#include "mlir/Dialect/OpenACC/OpenACC.h"
#include "mlir/IR/Builders.h"
#include "mlir/IR/BuiltinOps.h"
#include "mlir/Pass/Pass.h"
#include "mlir/Support/LLVM.h"
+#include "flang/Optimizer/Dialect/FIRDialect.h"
+#include "flang/Optimizer/HLFIR/HLFIRDialect.h"
#include "flang/Optimizer/Support/DataLayout.h"
+#include "mlir/Dialect/DLTI/DLTI.h"
using namespace mlir;
@@ -25,6 +29,11 @@ struct TestFIROpenACCInterfaces
StringRef getDescription() const final {
return "Test FIR implementation of the OpenACC interfaces.";
}
+ void getDependentDialects(::mlir::DialectRegistry ®istry) const override {
+ registry.insert<fir::FIROpsDialect, hlfir::hlfirDialect,
+ mlir::arith::ArithDialect, mlir::acc::OpenACCDialect,
+ mlir::DLTIDialect>();
+ }
void runOnOperation() override {
mlir::ModuleOp mod = getOperation();
auto datalayout =
|
|
@llvm/pr-subscribers-openacc Author: Razvan Lupusoru (razvanlupusoru) Changesfir.class is treated similarly as fir.box - but it has one key distinction which is that it doesn't hold an element type. Thus the categorization logic was mishandling this case for this reason (and also the fact that it assumed that a base object is always a fir.ref). This PR improves this handling and adds appropriate test exercising both a class and a class field to ensure categorization works. Full diff: https://github.com/llvm/llvm-project/pull/146174.diff 3 Files Affected:
diff --git a/flang/lib/Optimizer/OpenACC/FIROpenACCTypeInterfaces.cpp b/flang/lib/Optimizer/OpenACC/FIROpenACCTypeInterfaces.cpp
index 673d7e86c7ba0..2702f7e8c185e 100644
--- a/flang/lib/Optimizer/OpenACC/FIROpenACCTypeInterfaces.cpp
+++ b/flang/lib/Optimizer/OpenACC/FIROpenACCTypeInterfaces.cpp
@@ -320,8 +320,13 @@ template <>
mlir::acc::VariableTypeCategory
OpenACCMappableModel<fir::BaseBoxType>::getTypeCategory(mlir::Type type,
mlir::Value var) const {
+ // Class-type does not behave like a normal box because it does not hold an
+ // element type. Thus special handle it here.
+ if (mlir::isa<fir::ClassType>(type))
+ return mlir::acc::VariableTypeCategory::composite;
mlir::Type eleTy = fir::dyn_cast_ptrOrBoxEleTy(type);
+ assert(eleTy && "expect to be able to unwrap the element type");
// If the type enclosed by the box is a mappable type, then have it
// provide the type category.
@@ -346,7 +351,7 @@ OpenACCMappableModel<fir::BaseBoxType>::getTypeCategory(mlir::Type type,
return mlir::acc::VariableTypeCategory::nonscalar;
}
-static mlir::TypedValue<mlir::acc::PointerLikeType>
+static mlir::Value
getBaseRef(mlir::TypedValue<mlir::acc::PointerLikeType> varPtr) {
// If there is no defining op - the unwrapped reference is the base one.
mlir::Operation *op = varPtr.getDefiningOp();
@@ -372,7 +377,7 @@ getBaseRef(mlir::TypedValue<mlir::acc::PointerLikeType> varPtr) {
})
.Default([&](mlir::Operation *) { return varPtr; });
- return mlir::cast<mlir::TypedValue<mlir::acc::PointerLikeType>>(baseRef);
+ return baseRef;
}
static mlir::acc::VariableTypeCategory
@@ -384,10 +389,17 @@ categorizePointee(mlir::Type pointer,
// value would both be represented as !fir.ref<f32>. We do not want to treat
// such a reference as a scalar. Thus unwrap interior pointer calculations.
auto baseRef = getBaseRef(varPtr);
- mlir::Type eleTy = baseRef.getType().getElementType();
- if (auto mappableTy = mlir::dyn_cast<mlir::acc::MappableType>(eleTy))
- return mappableTy.getTypeCategory(varPtr);
+ if (auto mappableTy =
+ mlir::dyn_cast<mlir::acc::MappableType>(baseRef.getType()))
+ return mappableTy.getTypeCategory(baseRef);
+
+ // It must be a pointer-like type since it is not a MappableType.
+ auto ptrLikeTy = mlir::cast<mlir::acc::PointerLikeType>(baseRef.getType());
+ mlir::Type eleTy = ptrLikeTy.getElementType();
+
+ if (auto mappableEleTy = mlir::dyn_cast<mlir::acc::MappableType>(eleTy))
+ return mappableEleTy.getTypeCategory(varPtr);
if (isScalarLike(eleTy))
return mlir::acc::VariableTypeCategory::scalar;
diff --git a/flang/test/Fir/OpenACC/openacc-type-categories-class.f90 b/flang/test/Fir/OpenACC/openacc-type-categories-class.f90
new file mode 100644
index 0000000000000..0a38ab96a0315
--- /dev/null
+++ b/flang/test/Fir/OpenACC/openacc-type-categories-class.f90
@@ -0,0 +1,18 @@
+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)
+ end subroutine
+end module
+
+! RUN: bbc -fopenacc -emit-hlfir %s -o - | fir-opt -pass-pipeline='builtin.module(test-fir-openacc-interfaces)' --mlir-disable-threading 2>&1 | FileCheck %s
+! 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
diff --git a/flang/test/lib/OpenACC/TestOpenACCInterfaces.cpp b/flang/test/lib/OpenACC/TestOpenACCInterfaces.cpp
index 90aabd7d40d44..11567d1c0c6a3 100644
--- a/flang/test/lib/OpenACC/TestOpenACCInterfaces.cpp
+++ b/flang/test/lib/OpenACC/TestOpenACCInterfaces.cpp
@@ -6,12 +6,16 @@
//
//===----------------------------------------------------------------------===//
+#include "mlir/Dialect/Arith/IR/Arith.h"
#include "mlir/Dialect/OpenACC/OpenACC.h"
#include "mlir/IR/Builders.h"
#include "mlir/IR/BuiltinOps.h"
#include "mlir/Pass/Pass.h"
#include "mlir/Support/LLVM.h"
+#include "flang/Optimizer/Dialect/FIRDialect.h"
+#include "flang/Optimizer/HLFIR/HLFIRDialect.h"
#include "flang/Optimizer/Support/DataLayout.h"
+#include "mlir/Dialect/DLTI/DLTI.h"
using namespace mlir;
@@ -25,6 +29,11 @@ struct TestFIROpenACCInterfaces
StringRef getDescription() const final {
return "Test FIR implementation of the OpenACC interfaces.";
}
+ void getDependentDialects(::mlir::DialectRegistry ®istry) const override {
+ registry.insert<fir::FIROpsDialect, hlfir::hlfirDialect,
+ mlir::arith::ArithDialect, mlir::acc::OpenACCDialect,
+ mlir::DLTIDialect>();
+ }
void runOnOperation() override {
mlir::ModuleOp mod = getOperation();
auto datalayout =
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
clementval
reviewed
Jun 27, 2025
Comment on lines
+7
to
+8
| class(polyty), intent(inout) :: this | ||
| !$acc enter data copyin(this, this%field) |
There was a problem hiding this comment.
Have you check if it works as well with an unlimited polymorphic dummy?
class(*), intent(inout) :: this
!$acc enter data copyin(this)
There was a problem hiding this comment.
It did not - and now I fixed it as I described below.
clementval
reviewed
Jun 27, 2025
| @@ -0,0 +1,18 @@ | |||
| module mm | |||
There was a problem hiding this comment.
This test should probably go in flang/test/Lower/OpenACC or starts from mlir directly.
There was a problem hiding this comment.
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.
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!
clementval
reviewed
Jun 27, 2025
| end subroutine | ||
| end module | ||
|
|
||
| ! RUN: bbc -fopenacc -emit-hlfir %s -o - | fir-opt -pass-pipeline='builtin.module(test-fir-openacc-interfaces)' --mlir-disable-threading 2>&1 | FileCheck %s |
There was a problem hiding this comment.
Any reason to not add the run line at the top?
There was a problem hiding this comment.
Fixed. It was on accident.
clementval
reviewed
Jun 27, 2025
Comment on lines
325
to
326
| if (mlir::isa<fir::ClassType>(type)) | ||
| return mlir::acc::VariableTypeCategory::composite; |
There was a problem hiding this comment.
Should we make a distinction for unlimited polymorphic that can be smth else than a composite?
There was a problem hiding this comment.
Done. I am currently categorizing it as "uncategorized". Although the OpenACC spec does not discuss unlimited polymorphic or assumed type - the language semantics should apply. Namely, the type should be appropriately selected for the categorization to work.
clementval
approved these changes
Jun 30, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/204/builds/13965 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/203/builds/15152 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/205/builds/13942 Here is the relevant piece of the build log for the reference |
|
I just sent out a fix for the build issues: #146431 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
fir.class is treated similarly as fir.box - but it has one key distinction which is that it doesn't hold an element type. Thus the categorization logic was mishandling this case for this reason (and also the fact that it assumed that a base object is always a fir.ref).
This PR improves this handling and adds appropriate test exercising both a class and a class field to ensure categorization works.