[Flang][OpenMP] Fix negative array indexing with allocatable derived type array maps #154193
+81
−22
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
agozillon
requested review from
DominikAdamski,
TIFitis,
abidh,
bhandarkar-pranav,
dpalermo,
ergawy,
jsjodin,
kparzysz,
mjklemm,
raghavendhra and
skatrak
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-openmp Author: None (agozillon) ChangesThe main problem is that the previous intermediate map generation for allocatable members wasn't quite handling negative bounds acccesses correctly, it seems to require slightly more complicated access using shape_shift/dimension information. So this more closely mimics what Flang generates in other cases now. There is still a path for non-Box types to go down the old route for the moment, so it is possible we may still have issues with negative bounds in these cases. But, that's better in another PR if we come across it, instead of too much change in this one. Full diff: https://github.com/llvm/llvm-project/pull/154193.diff 3 Files Affected:
diff --git a/flang/lib/Lower/OpenMP/Utils.cpp b/flang/lib/Lower/OpenMP/Utils.cpp
index 77b1e39083aa6..22ab0f4bca163 100644
--- a/flang/lib/Lower/OpenMP/Utils.cpp
+++ b/flang/lib/Lower/OpenMP/Utils.cpp
@@ -13,7 +13,6 @@
#include "Utils.h"
#include "ClauseFinder.h"
-#include "flang/Lower/OpenMP/Clauses.h"
#include <flang/Lower/AbstractConverter.h>
#include <flang/Lower/ConvertType.h>
#include <flang/Lower/DirectivesCommon.h>
@@ -189,16 +188,11 @@ static void generateArrayIndices(lower::AbstractConverter &converter,
for (auto v : arr->subscript()) {
if (std::holds_alternative<Triplet>(v.u))
TODO(clauseLocation, "Triplet indexing in map clause is unsupported");
-
auto expr = std::get<Fortran::evaluate::IndirectSubscriptIntegerExpr>(v.u);
mlir::Value subscript =
fir::getBase(converter.genExprValue(toEvExpr(expr.value()), stmtCtx));
- mlir::Value one = firOpBuilder.createIntegerConstant(
- clauseLocation, firOpBuilder.getIndexType(), 1);
- subscript = firOpBuilder.createConvert(
- clauseLocation, firOpBuilder.getIndexType(), subscript);
- indices.push_back(mlir::arith::SubIOp::create(firOpBuilder, clauseLocation,
- subscript, one));
+ indices.push_back(firOpBuilder.createConvert(
+ clauseLocation, firOpBuilder.getIndexType(), subscript));
}
}
@@ -331,10 +325,41 @@ mlir::Value createParentSymAndGenIntermediateMaps(
subscriptIndices, objectList[i]);
assert(!subscriptIndices.empty() &&
"missing expected indices for map clause");
- curValue = fir::CoordinateOp::create(
- firOpBuilder, clauseLocation,
- firOpBuilder.getRefType(arrType.getEleTy()), curValue,
- subscriptIndices);
+ if (auto boxTy = llvm::dyn_cast<fir::BaseBoxType>(curValue.getType())) {
+ fir::ExtendedValue exv =
+ hlfir::translateToExtendedValue(clauseLocation, firOpBuilder,
+ hlfir::Entity{curValue},
+ /*contiguousHint=*/
+ true)
+ .first;
+
+ mlir::Type idxTy = firOpBuilder.getIndexType();
+ llvm::SmallVector<mlir::Value> shiftOperands;
+ for (unsigned dim = 0; dim < exv.rank(); ++dim) {
+ mlir::Value d =
+ firOpBuilder.createIntegerConstant(clauseLocation, idxTy, dim);
+ auto dimInfo = fir::BoxDimsOp::create(
+ firOpBuilder, clauseLocation, idxTy, idxTy, idxTy, curValue, d);
+ shiftOperands.push_back(dimInfo.getLowerBound());
+ shiftOperands.push_back(dimInfo.getExtent());
+ }
+ auto shapeShiftType =
+ fir::ShapeShiftType::get(firOpBuilder.getContext(), exv.rank());
+ mlir::Value shapeShift = fir::ShapeShiftOp::create(
+ firOpBuilder, clauseLocation, shapeShiftType, shiftOperands);
+ auto addrOp =
+ fir::BoxAddrOp::create(firOpBuilder, clauseLocation, curValue);
+ curValue = fir::ArrayCoorOp::create(
+ firOpBuilder, clauseLocation,
+ firOpBuilder.getRefType(arrType.getEleTy()), addrOp, shapeShift,
+ /*slice=*/mlir::Value{}, subscriptIndices,
+ /*typeparms=*/mlir::ValueRange{});
+ } else {
+ curValue = fir::CoordinateOp::create(
+ firOpBuilder, clauseLocation,
+ firOpBuilder.getRefType(arrType.getEleTy()), curValue,
+ subscriptIndices);
+ }
}
}
@@ -424,7 +449,6 @@ mlir::Value createParentSymAndGenIntermediateMaps(
currentIndicesIdx++;
}
}
-
return curValue;
}
diff --git a/flang/test/Integration/OpenMP/map-types-and-sizes.f90 b/flang/test/Integration/OpenMP/map-types-and-sizes.f90
index 665be5a8db4d4..44a049f5ac510 100644
--- a/flang/test/Integration/OpenMP/map-types-and-sizes.f90
+++ b/flang/test/Integration/OpenMP/map-types-and-sizes.f90
@@ -785,15 +785,20 @@ end subroutine mapType_common_block_members
!CHECK: %[[BOUNDS_CALC:.*]] = sub i64 %[[BOUNDS_LD_2]], 1
!CHECK: %[[OFF_PTR_CALC_0:.*]] = sub i64 %[[BOUNDS_LD]], 1
!CHECK: %[[OFF_PTR_2:.*]] = getelementptr { ptr, i64, i32, i8, i8, i8, i8, [1 x [3 x i64]], ptr, [1 x i64] }, ptr %[[OFF_PTR_1]], i32 0, i32 0
+!CHECK: %[[GEP_LB:.*]] = getelementptr { ptr, i64, i32, i8, i8, i8, i8, [1 x [3 x i64]], ptr, [1 x i64] }, ptr %[[ALLOCA_0]], i32 0, i32 7, i64 0, i32 0
+!CHECK: %[[LOAD_LB:.*]] = load i64, ptr %[[GEP_LB]], align 8
+!CHECK: %[[GEP_UB:.*]] = getelementptr { ptr, i64, i32, i8, i8, i8, i8, [1 x [3 x i64]], ptr, [1 x i64] }, ptr %[[ALLOCA_0]], i32 0, i32 7, i64 0, i32 1
+!CHECK: %[[LOAD_UB:.*]] = load i64, ptr %[[GEP_UB]], align 8
!CHECK: %[[GEP_DESC_PTR:.*]] = getelementptr { ptr, i64, i32, i8, i8, i8, i8, [1 x [3 x i64]], ptr, [1 x i64] }, ptr %[[ALLOCA_0]], i32 0, i32 0
-!CHECK: %[[LOAD_DESC_PTR:.*]] = load ptr, ptr %[[GEP_DESC_PTR]], align 8
-!CHECK: %[[SZ_CALC_1:.*]] = getelementptr { ptr, i64, i32, i8, i8, i8, i8, [1 x [3 x i64]], ptr, [1 x i64] }, ptr %[[ALLOCA_0]], i32 0, i32 7, i32 0, i32 2
-!CHECK: %[[SZ_CALC_2:.*]] = load i64, ptr %[[SZ_CALC_1]], align 8
-!CHECK: %[[SZ_CALC_3:.*]] = mul nsw i64 1, %[[SZ_CALC_2]]
-!CHECK: %[[SZ_CALC_4:.*]] = add nsw i64 %[[SZ_CALC_3]], 0
-!CHECK: %[[SZ_CALC_5:.*]] = getelementptr i8, ptr %[[LOAD_DESC_PTR]], i64 %[[SZ_CALC_4]]
-!CHECK: %[[SZ_CALC_6:.*]] = getelementptr %_QFmaptype_nested_derived_type_member_idxTvertexes, ptr %[[SZ_CALC_5]], i32 0, i32 2
-!CHECK: %[[OFF_PTR_4:.*]] = getelementptr { ptr, i64, i32, i8, i8, i8, i8, [1 x [3 x i64]] }, ptr %[[SZ_CALC_6]], i32 0, i32 0
+!CHECK: %[[SZ_CALC_1:.*]] = load ptr, ptr %[[GEP_DESC_PTR]], align 8
+!CHECK: %[[SZ_CALC_2:.*]] = sub nsw i64 2, %[[LOAD_LB]]
+!CHECK: %[[SZ_CALC_3:.*]] = mul nsw i64 %[[SZ_CALC_2]], 1
+!CHECK: %[[SZ_CALC_4:.*]] = mul nsw i64 %[[SZ_CALC_3]], 1
+!CHECK: %[[SZ_CALC_5:.*]] = add nsw i64 %[[SZ_CALC_4]], 0
+!CHECK: %[[SZ_CALC_6:.*]] = mul nsw i64 1, %[[LOAD_UB]]
+!CHECK: %[[SZ_CALC_7:.*]] = getelementptr %_QFmaptype_nested_derived_type_member_idxTvertexes, ptr %[[SZ_CALC_1]], i64 %[[SZ_CALC_5]]
+!CHECK: %[[SZ_CALC_8:.*]] = getelementptr %_QFmaptype_nested_derived_type_member_idxTvertexes, ptr %[[SZ_CALC_7]], i32 0, i32 2
+!CHECK: %[[OFF_PTR_4:.*]] = getelementptr { ptr, i64, i32, i8, i8, i8, i8, [1 x [3 x i64]] }, ptr %[[SZ_CALC_8]], i32 0, i32 0
!CHECK: %[[OFF_PTR_CALC_1:.*]] = sub i64 %[[OFF_PTR_CALC_0]], 0
!CHECK: %[[OFF_PTR_CALC_2:.*]] = add i64 %[[OFF_PTR_CALC_1]], 1
!CHECK: %[[OFF_PTR_CALC_3:.*]] = mul i64 1, %[[OFF_PTR_CALC_2]]
@@ -838,7 +843,7 @@ end subroutine mapType_common_block_members
!CHECK: %[[BASE_PTR_ARR:.*]] = getelementptr inbounds [7 x ptr], ptr %.offload_baseptrs, i32 0, i32 4
!CHECK: store ptr %[[BASE_PTR_1]], ptr %[[BASE_PTR_ARR]], align 8
!CHECK: %[[OFFLOAD_PTR_ARR:.*]] = getelementptr inbounds [7 x ptr], ptr %.offload_ptrs, i32 0, i32 4
-!CHECK: store ptr %[[SZ_CALC_6]], ptr %[[OFFLOAD_PTR_ARR]], align 8
+!CHECK: store ptr %[[SZ_CALC_8]], ptr %[[OFFLOAD_PTR_ARR]], align 8
!CHECK: %[[BASE_PTR_ARR:.*]] = getelementptr inbounds [7 x ptr], ptr %.offload_baseptrs, i32 0, i32 5
!CHECK: store ptr %[[BASE_PTR_1]], ptr %[[BASE_PTR_ARR]], align 8
!CHECK: %[[OFFLOAD_PTR_ARR:.*]] = getelementptr inbounds [7 x ptr], ptr %.offload_ptrs, i32 0, i32 5
diff --git a/flang/test/Lower/OpenMP/map-neg-alloca-derived-type-array.f90 b/flang/test/Lower/OpenMP/map-neg-alloca-derived-type-array.f90
new file mode 100644
index 0000000000000..3ff7bd1817704
--- /dev/null
+++ b/flang/test/Lower/OpenMP/map-neg-alloca-derived-type-array.f90
@@ -0,0 +1,31 @@
+!RUN: %flang_fc1 -emit-hlfir -fopenmp %s -o - | FileCheck %s
+
+subroutine map_negative_bounds_allocatable_dtype()
+ type derived_type
+ real(4), pointer :: data(:,:,:) => null()
+ end type
+ type(derived_type), allocatable :: dtype(:,:)
+
+ !$omp target map(tofrom: dtype(-1,1)%data)
+ dtype(-1,1)%data(1,1,1) = 10
+ !$omp end target
+end subroutine
+
+! CHECK-LABEL: func.func @_QPmap_negative_bounds_allocatable_dtype() {
+! CHECK: %{{.*}} = fir.load %{{.*}}#0 : !fir.ref<!fir.box<!fir.heap<!fir.array<?x?x!fir.type<_QFmap_negative_bounds_allocatable_dtypeTderived_type{data:!fir.box<!fir.ptr<!fir.array<?x?x?xf32>>>}>>>>>
+! CHECK: %[[VAL_0:.*]] = fir.load %{{.*}}#0 : !fir.ref<!fir.box<!fir.heap<!fir.array<?x?x!fir.type<_QFmap_negative_bounds_allocatable_dtypeTderived_type{data:!fir.box<!fir.ptr<!fir.array<?x?x?xf32>>>}>>>>>
+! CHECK: %[[VAL_1:.*]] = arith.constant -1 : i64
+! CHECK: %[[VAL_2:.*]] = fir.convert %[[VAL_1]] : (i64) -> index
+! CHECK: %[[VAL_3:.*]] = arith.constant 1 : i64
+! CHECK: %[[VAL_4:.*]] = fir.convert %[[VAL_3]] : (i64) -> index
+! CHECK: %[[VAL_5:.*]] = arith.constant 0 : index
+! CHECK: %[[VAL_6:.*]]:3 = fir.box_dims %[[VAL_0]], %[[VAL_5]] : (!fir.box<!fir.heap<!fir.array<?x?x!fir.type<_QFmap_negative_bounds_allocatable_dtypeTderived_type{data:!fir.box<!fir.ptr<!fir.array<?x?x?xf32>>>}>>>>, index) -> (index, index, index)
+! CHECK: %[[VAL_7:.*]] = arith.constant 1 : index
+! CHECK: %[[VAL_8:.*]]:3 = fir.box_dims %[[VAL_0]], %[[VAL_7]] : (!fir.box<!fir.heap<!fir.array<?x?x!fir.type<_QFmap_negative_bounds_allocatable_dtypeTderived_type{data:!fir.box<!fir.ptr<!fir.array<?x?x?xf32>>>}>>>>, index) -> (index, index, index)
+! CHECK: %[[VAL_9:.*]] = fir.shape_shift %[[VAL_6]]#0, %[[VAL_6]]#1, %[[VAL_8]]#0, %[[VAL_8]]#1 : (index, index, index, index) -> !fir.shapeshift<2>
+! CHECK: %[[VAL_10:.*]] = fir.box_addr %[[VAL_0]] : (!fir.box<!fir.heap<!fir.array<?x?x!fir.type<_QFmap_negative_bounds_allocatable_dtypeTderived_type{data:!fir.box<!fir.ptr<!fir.array<?x?x?xf32>>>}>>>>) -> !fir.heap<!fir.array<?x?x!fir.type<_QFmap_negative_bounds_allocatable_dtypeTderived_type{data:!fir.box<!fir.ptr<!fir.array<?x?x?xf32>>>}>>>
+! CHECK: %[[VAL_11:.*]] = fir.array_coor %[[VAL_10]](%[[VAL_9]]) %[[VAL_2]], %[[VAL_4]] : (!fir.heap<!fir.array<?x?x!fir.type<_QFmap_negative_bounds_allocatable_dtypeTderived_type{data:!fir.box<!fir.ptr<!fir.array<?x?x?xf32>>>}>>>, !fir.shapeshift<2>, index, index) -> !fir.ref<!fir.type<_QFmap_negative_bounds_allocatable_dtypeTderived_type{data:!fir.box<!fir.ptr<!fir.array<?x?x?xf32>>>}>>
+! CHECK: %[[VAL_12:.*]] = fir.coordinate_of %[[VAL_11]], data : (!fir.ref<!fir.type<_QFmap_negative_bounds_allocatable_dtypeTderived_type{data:!fir.box<!fir.ptr<!fir.array<?x?x?xf32>>>}>>) -> !fir.ref<!fir.box<!fir.ptr<!fir.array<?x?x?xf32>>>>
+! CHECK: %[[VAL_13:.*]] = fir.box_offset %[[VAL_12]] base_addr : (!fir.ref<!fir.box<!fir.ptr<!fir.array<?x?x?xf32>>>>) -> !fir.llvm_ptr<!fir.ref<!fir.array<?x?x?xf32>>>
+! CHECK: %[[VAL_14:.*]] = omp.map.info var_ptr(%[[VAL_12]] : !fir.ref<!fir.box<!fir.ptr<!fir.array<?x?x?xf32>>>>, f32) map_clauses(tofrom) capture(ByRef) var_ptr_ptr(%[[VAL_13]] : !fir.llvm_ptr<!fir.ref<!fir.array<?x?x?xf32>>>) bounds({{.*}}) -> !fir.llvm_ptr<!fir.ref<!fir.array<?x?x?xf32>>> {name = ""}
+! CHECK: %[[VAL_15:.*]] = omp.map.info var_ptr(%[[VAL_12]] : !fir.ref<!fir.box<!fir.ptr<!fir.array<?x?x?xf32>>>>, !fir.box<!fir.ptr<!fir.array<?x?x?xf32>>>) map_clauses(to) capture(ByRef) -> !fir.ref<!fir.box<!fir.ptr<!fir.array<?x?x?xf32>>>> {name = {{.*}}}
|
|
@llvm/pr-subscribers-flang-fir-hlfir Author: None (agozillon) ChangesThe main problem is that the previous intermediate map generation for allocatable members wasn't quite handling negative bounds acccesses correctly, it seems to require slightly more complicated access using shape_shift/dimension information. So this more closely mimics what Flang generates in other cases now. There is still a path for non-Box types to go down the old route for the moment, so it is possible we may still have issues with negative bounds in these cases. But, that's better in another PR if we come across it, instead of too much change in this one. Full diff: https://github.com/llvm/llvm-project/pull/154193.diff 3 Files Affected:
diff --git a/flang/lib/Lower/OpenMP/Utils.cpp b/flang/lib/Lower/OpenMP/Utils.cpp
index 77b1e39083aa6..22ab0f4bca163 100644
--- a/flang/lib/Lower/OpenMP/Utils.cpp
+++ b/flang/lib/Lower/OpenMP/Utils.cpp
@@ -13,7 +13,6 @@
#include "Utils.h"
#include "ClauseFinder.h"
-#include "flang/Lower/OpenMP/Clauses.h"
#include <flang/Lower/AbstractConverter.h>
#include <flang/Lower/ConvertType.h>
#include <flang/Lower/DirectivesCommon.h>
@@ -189,16 +188,11 @@ static void generateArrayIndices(lower::AbstractConverter &converter,
for (auto v : arr->subscript()) {
if (std::holds_alternative<Triplet>(v.u))
TODO(clauseLocation, "Triplet indexing in map clause is unsupported");
-
auto expr = std::get<Fortran::evaluate::IndirectSubscriptIntegerExpr>(v.u);
mlir::Value subscript =
fir::getBase(converter.genExprValue(toEvExpr(expr.value()), stmtCtx));
- mlir::Value one = firOpBuilder.createIntegerConstant(
- clauseLocation, firOpBuilder.getIndexType(), 1);
- subscript = firOpBuilder.createConvert(
- clauseLocation, firOpBuilder.getIndexType(), subscript);
- indices.push_back(mlir::arith::SubIOp::create(firOpBuilder, clauseLocation,
- subscript, one));
+ indices.push_back(firOpBuilder.createConvert(
+ clauseLocation, firOpBuilder.getIndexType(), subscript));
}
}
@@ -331,10 +325,41 @@ mlir::Value createParentSymAndGenIntermediateMaps(
subscriptIndices, objectList[i]);
assert(!subscriptIndices.empty() &&
"missing expected indices for map clause");
- curValue = fir::CoordinateOp::create(
- firOpBuilder, clauseLocation,
- firOpBuilder.getRefType(arrType.getEleTy()), curValue,
- subscriptIndices);
+ if (auto boxTy = llvm::dyn_cast<fir::BaseBoxType>(curValue.getType())) {
+ fir::ExtendedValue exv =
+ hlfir::translateToExtendedValue(clauseLocation, firOpBuilder,
+ hlfir::Entity{curValue},
+ /*contiguousHint=*/
+ true)
+ .first;
+
+ mlir::Type idxTy = firOpBuilder.getIndexType();
+ llvm::SmallVector<mlir::Value> shiftOperands;
+ for (unsigned dim = 0; dim < exv.rank(); ++dim) {
+ mlir::Value d =
+ firOpBuilder.createIntegerConstant(clauseLocation, idxTy, dim);
+ auto dimInfo = fir::BoxDimsOp::create(
+ firOpBuilder, clauseLocation, idxTy, idxTy, idxTy, curValue, d);
+ shiftOperands.push_back(dimInfo.getLowerBound());
+ shiftOperands.push_back(dimInfo.getExtent());
+ }
+ auto shapeShiftType =
+ fir::ShapeShiftType::get(firOpBuilder.getContext(), exv.rank());
+ mlir::Value shapeShift = fir::ShapeShiftOp::create(
+ firOpBuilder, clauseLocation, shapeShiftType, shiftOperands);
+ auto addrOp =
+ fir::BoxAddrOp::create(firOpBuilder, clauseLocation, curValue);
+ curValue = fir::ArrayCoorOp::create(
+ firOpBuilder, clauseLocation,
+ firOpBuilder.getRefType(arrType.getEleTy()), addrOp, shapeShift,
+ /*slice=*/mlir::Value{}, subscriptIndices,
+ /*typeparms=*/mlir::ValueRange{});
+ } else {
+ curValue = fir::CoordinateOp::create(
+ firOpBuilder, clauseLocation,
+ firOpBuilder.getRefType(arrType.getEleTy()), curValue,
+ subscriptIndices);
+ }
}
}
@@ -424,7 +449,6 @@ mlir::Value createParentSymAndGenIntermediateMaps(
currentIndicesIdx++;
}
}
-
return curValue;
}
diff --git a/flang/test/Integration/OpenMP/map-types-and-sizes.f90 b/flang/test/Integration/OpenMP/map-types-and-sizes.f90
index 665be5a8db4d4..44a049f5ac510 100644
--- a/flang/test/Integration/OpenMP/map-types-and-sizes.f90
+++ b/flang/test/Integration/OpenMP/map-types-and-sizes.f90
@@ -785,15 +785,20 @@ end subroutine mapType_common_block_members
!CHECK: %[[BOUNDS_CALC:.*]] = sub i64 %[[BOUNDS_LD_2]], 1
!CHECK: %[[OFF_PTR_CALC_0:.*]] = sub i64 %[[BOUNDS_LD]], 1
!CHECK: %[[OFF_PTR_2:.*]] = getelementptr { ptr, i64, i32, i8, i8, i8, i8, [1 x [3 x i64]], ptr, [1 x i64] }, ptr %[[OFF_PTR_1]], i32 0, i32 0
+!CHECK: %[[GEP_LB:.*]] = getelementptr { ptr, i64, i32, i8, i8, i8, i8, [1 x [3 x i64]], ptr, [1 x i64] }, ptr %[[ALLOCA_0]], i32 0, i32 7, i64 0, i32 0
+!CHECK: %[[LOAD_LB:.*]] = load i64, ptr %[[GEP_LB]], align 8
+!CHECK: %[[GEP_UB:.*]] = getelementptr { ptr, i64, i32, i8, i8, i8, i8, [1 x [3 x i64]], ptr, [1 x i64] }, ptr %[[ALLOCA_0]], i32 0, i32 7, i64 0, i32 1
+!CHECK: %[[LOAD_UB:.*]] = load i64, ptr %[[GEP_UB]], align 8
!CHECK: %[[GEP_DESC_PTR:.*]] = getelementptr { ptr, i64, i32, i8, i8, i8, i8, [1 x [3 x i64]], ptr, [1 x i64] }, ptr %[[ALLOCA_0]], i32 0, i32 0
-!CHECK: %[[LOAD_DESC_PTR:.*]] = load ptr, ptr %[[GEP_DESC_PTR]], align 8
-!CHECK: %[[SZ_CALC_1:.*]] = getelementptr { ptr, i64, i32, i8, i8, i8, i8, [1 x [3 x i64]], ptr, [1 x i64] }, ptr %[[ALLOCA_0]], i32 0, i32 7, i32 0, i32 2
-!CHECK: %[[SZ_CALC_2:.*]] = load i64, ptr %[[SZ_CALC_1]], align 8
-!CHECK: %[[SZ_CALC_3:.*]] = mul nsw i64 1, %[[SZ_CALC_2]]
-!CHECK: %[[SZ_CALC_4:.*]] = add nsw i64 %[[SZ_CALC_3]], 0
-!CHECK: %[[SZ_CALC_5:.*]] = getelementptr i8, ptr %[[LOAD_DESC_PTR]], i64 %[[SZ_CALC_4]]
-!CHECK: %[[SZ_CALC_6:.*]] = getelementptr %_QFmaptype_nested_derived_type_member_idxTvertexes, ptr %[[SZ_CALC_5]], i32 0, i32 2
-!CHECK: %[[OFF_PTR_4:.*]] = getelementptr { ptr, i64, i32, i8, i8, i8, i8, [1 x [3 x i64]] }, ptr %[[SZ_CALC_6]], i32 0, i32 0
+!CHECK: %[[SZ_CALC_1:.*]] = load ptr, ptr %[[GEP_DESC_PTR]], align 8
+!CHECK: %[[SZ_CALC_2:.*]] = sub nsw i64 2, %[[LOAD_LB]]
+!CHECK: %[[SZ_CALC_3:.*]] = mul nsw i64 %[[SZ_CALC_2]], 1
+!CHECK: %[[SZ_CALC_4:.*]] = mul nsw i64 %[[SZ_CALC_3]], 1
+!CHECK: %[[SZ_CALC_5:.*]] = add nsw i64 %[[SZ_CALC_4]], 0
+!CHECK: %[[SZ_CALC_6:.*]] = mul nsw i64 1, %[[LOAD_UB]]
+!CHECK: %[[SZ_CALC_7:.*]] = getelementptr %_QFmaptype_nested_derived_type_member_idxTvertexes, ptr %[[SZ_CALC_1]], i64 %[[SZ_CALC_5]]
+!CHECK: %[[SZ_CALC_8:.*]] = getelementptr %_QFmaptype_nested_derived_type_member_idxTvertexes, ptr %[[SZ_CALC_7]], i32 0, i32 2
+!CHECK: %[[OFF_PTR_4:.*]] = getelementptr { ptr, i64, i32, i8, i8, i8, i8, [1 x [3 x i64]] }, ptr %[[SZ_CALC_8]], i32 0, i32 0
!CHECK: %[[OFF_PTR_CALC_1:.*]] = sub i64 %[[OFF_PTR_CALC_0]], 0
!CHECK: %[[OFF_PTR_CALC_2:.*]] = add i64 %[[OFF_PTR_CALC_1]], 1
!CHECK: %[[OFF_PTR_CALC_3:.*]] = mul i64 1, %[[OFF_PTR_CALC_2]]
@@ -838,7 +843,7 @@ end subroutine mapType_common_block_members
!CHECK: %[[BASE_PTR_ARR:.*]] = getelementptr inbounds [7 x ptr], ptr %.offload_baseptrs, i32 0, i32 4
!CHECK: store ptr %[[BASE_PTR_1]], ptr %[[BASE_PTR_ARR]], align 8
!CHECK: %[[OFFLOAD_PTR_ARR:.*]] = getelementptr inbounds [7 x ptr], ptr %.offload_ptrs, i32 0, i32 4
-!CHECK: store ptr %[[SZ_CALC_6]], ptr %[[OFFLOAD_PTR_ARR]], align 8
+!CHECK: store ptr %[[SZ_CALC_8]], ptr %[[OFFLOAD_PTR_ARR]], align 8
!CHECK: %[[BASE_PTR_ARR:.*]] = getelementptr inbounds [7 x ptr], ptr %.offload_baseptrs, i32 0, i32 5
!CHECK: store ptr %[[BASE_PTR_1]], ptr %[[BASE_PTR_ARR]], align 8
!CHECK: %[[OFFLOAD_PTR_ARR:.*]] = getelementptr inbounds [7 x ptr], ptr %.offload_ptrs, i32 0, i32 5
diff --git a/flang/test/Lower/OpenMP/map-neg-alloca-derived-type-array.f90 b/flang/test/Lower/OpenMP/map-neg-alloca-derived-type-array.f90
new file mode 100644
index 0000000000000..3ff7bd1817704
--- /dev/null
+++ b/flang/test/Lower/OpenMP/map-neg-alloca-derived-type-array.f90
@@ -0,0 +1,31 @@
+!RUN: %flang_fc1 -emit-hlfir -fopenmp %s -o - | FileCheck %s
+
+subroutine map_negative_bounds_allocatable_dtype()
+ type derived_type
+ real(4), pointer :: data(:,:,:) => null()
+ end type
+ type(derived_type), allocatable :: dtype(:,:)
+
+ !$omp target map(tofrom: dtype(-1,1)%data)
+ dtype(-1,1)%data(1,1,1) = 10
+ !$omp end target
+end subroutine
+
+! CHECK-LABEL: func.func @_QPmap_negative_bounds_allocatable_dtype() {
+! CHECK: %{{.*}} = fir.load %{{.*}}#0 : !fir.ref<!fir.box<!fir.heap<!fir.array<?x?x!fir.type<_QFmap_negative_bounds_allocatable_dtypeTderived_type{data:!fir.box<!fir.ptr<!fir.array<?x?x?xf32>>>}>>>>>
+! CHECK: %[[VAL_0:.*]] = fir.load %{{.*}}#0 : !fir.ref<!fir.box<!fir.heap<!fir.array<?x?x!fir.type<_QFmap_negative_bounds_allocatable_dtypeTderived_type{data:!fir.box<!fir.ptr<!fir.array<?x?x?xf32>>>}>>>>>
+! CHECK: %[[VAL_1:.*]] = arith.constant -1 : i64
+! CHECK: %[[VAL_2:.*]] = fir.convert %[[VAL_1]] : (i64) -> index
+! CHECK: %[[VAL_3:.*]] = arith.constant 1 : i64
+! CHECK: %[[VAL_4:.*]] = fir.convert %[[VAL_3]] : (i64) -> index
+! CHECK: %[[VAL_5:.*]] = arith.constant 0 : index
+! CHECK: %[[VAL_6:.*]]:3 = fir.box_dims %[[VAL_0]], %[[VAL_5]] : (!fir.box<!fir.heap<!fir.array<?x?x!fir.type<_QFmap_negative_bounds_allocatable_dtypeTderived_type{data:!fir.box<!fir.ptr<!fir.array<?x?x?xf32>>>}>>>>, index) -> (index, index, index)
+! CHECK: %[[VAL_7:.*]] = arith.constant 1 : index
+! CHECK: %[[VAL_8:.*]]:3 = fir.box_dims %[[VAL_0]], %[[VAL_7]] : (!fir.box<!fir.heap<!fir.array<?x?x!fir.type<_QFmap_negative_bounds_allocatable_dtypeTderived_type{data:!fir.box<!fir.ptr<!fir.array<?x?x?xf32>>>}>>>>, index) -> (index, index, index)
+! CHECK: %[[VAL_9:.*]] = fir.shape_shift %[[VAL_6]]#0, %[[VAL_6]]#1, %[[VAL_8]]#0, %[[VAL_8]]#1 : (index, index, index, index) -> !fir.shapeshift<2>
+! CHECK: %[[VAL_10:.*]] = fir.box_addr %[[VAL_0]] : (!fir.box<!fir.heap<!fir.array<?x?x!fir.type<_QFmap_negative_bounds_allocatable_dtypeTderived_type{data:!fir.box<!fir.ptr<!fir.array<?x?x?xf32>>>}>>>>) -> !fir.heap<!fir.array<?x?x!fir.type<_QFmap_negative_bounds_allocatable_dtypeTderived_type{data:!fir.box<!fir.ptr<!fir.array<?x?x?xf32>>>}>>>
+! CHECK: %[[VAL_11:.*]] = fir.array_coor %[[VAL_10]](%[[VAL_9]]) %[[VAL_2]], %[[VAL_4]] : (!fir.heap<!fir.array<?x?x!fir.type<_QFmap_negative_bounds_allocatable_dtypeTderived_type{data:!fir.box<!fir.ptr<!fir.array<?x?x?xf32>>>}>>>, !fir.shapeshift<2>, index, index) -> !fir.ref<!fir.type<_QFmap_negative_bounds_allocatable_dtypeTderived_type{data:!fir.box<!fir.ptr<!fir.array<?x?x?xf32>>>}>>
+! CHECK: %[[VAL_12:.*]] = fir.coordinate_of %[[VAL_11]], data : (!fir.ref<!fir.type<_QFmap_negative_bounds_allocatable_dtypeTderived_type{data:!fir.box<!fir.ptr<!fir.array<?x?x?xf32>>>}>>) -> !fir.ref<!fir.box<!fir.ptr<!fir.array<?x?x?xf32>>>>
+! CHECK: %[[VAL_13:.*]] = fir.box_offset %[[VAL_12]] base_addr : (!fir.ref<!fir.box<!fir.ptr<!fir.array<?x?x?xf32>>>>) -> !fir.llvm_ptr<!fir.ref<!fir.array<?x?x?xf32>>>
+! CHECK: %[[VAL_14:.*]] = omp.map.info var_ptr(%[[VAL_12]] : !fir.ref<!fir.box<!fir.ptr<!fir.array<?x?x?xf32>>>>, f32) map_clauses(tofrom) capture(ByRef) var_ptr_ptr(%[[VAL_13]] : !fir.llvm_ptr<!fir.ref<!fir.array<?x?x?xf32>>>) bounds({{.*}}) -> !fir.llvm_ptr<!fir.ref<!fir.array<?x?x?xf32>>> {name = ""}
+! CHECK: %[[VAL_15:.*]] = omp.map.info var_ptr(%[[VAL_12]] : !fir.ref<!fir.box<!fir.ptr<!fir.array<?x?x?xf32>>>>, !fir.box<!fir.ptr<!fir.array<?x?x?xf32>>>) map_clauses(to) capture(ByRef) -> !fir.ref<!fir.box<!fir.ptr<!fir.array<?x?x?xf32>>>> {name = {{.*}}}
|
kparzysz
reviewed
Aug 19, 2025
flang/lib/Lower/OpenMP/Utils.cpp
Outdated
There was a problem hiding this comment.
This is still needed. For example, old line 45 uses clause::Collapse.
There was a problem hiding this comment.
Seems to build happily without it, likely due to picking it up from one of the other headers, but happy to re-add it!
There was a problem hiding this comment.
Moved it below with the rest of the includes, just to unify them a bit more, happy to change it back though if it's not the right convention (just seemed to be a bit more inline with the rest)
flang/lib/Lower/OpenMP/Utils.cpp
Outdated
There was a problem hiding this comment.
We used to subtract 1 from subscript indices in this case. Was that always wrong?
There was a problem hiding this comment.
It's definitely incorrect in the box case, as it'll result in runtime errors. In the regular CoordinateOp case it doesn't result in any (obvious from the tests I have and we have downstream) issues with or without it, I would be happy to re-add it though in the else section which would likely be the more prudent thing to do until I can work out a test-case that'd show which is flawed. I originally added the subtraction as the bit of IR I was looking at to work out the appropriate operations made use of it, so it seems like it should be the correct thing to do.
There was a problem hiding this comment.
Came up with a test for it, seems I manage to circumvent it in my current tests downstream! But yes, the subtraction is required for this branch, but not the other. So I'll be re-adding it in the next update, thank you for pointing it out!
There was a problem hiding this comment.
Re-added this little snippet in the below branch to achieve the previous correct behavior.
mjklemm
requested changes
Aug 20, 2025
flang/lib/Lower/OpenMP/Utils.cpp
Outdated
There was a problem hiding this comment.
Please document the maths behind the generated code in terms of a formula in a comment.
There was a problem hiding this comment.
I actually found a nice helper function that effectively does the same thing while looking into this a bit more :-) And I've done my best to comment this, someone with a bit more knowledge on HLFIR/FIR may be able to call me out on the details though.
agozillon
force-pushed
the
neg-bounds-map-error
branch
from
ef5c7ba to
c73d86a
Compare
agozillon
commented
Aug 22, 2025
flang/lib/Lower/OpenMP/Utils.cpp
Outdated
There was a problem hiding this comment.
Moved it below with the rest of the includes, just to unify them a bit more, happy to change it back though if it's not the right convention (just seemed to be a bit more inline with the rest)
flang/lib/Lower/OpenMP/Utils.cpp
Outdated
There was a problem hiding this comment.
Re-added this little snippet in the below branch to achieve the previous correct behavior.
flang/lib/Lower/OpenMP/Utils.cpp
Outdated
There was a problem hiding this comment.
I actually found a nice helper function that effectively does the same thing while looking into this a bit more :-) And I've done my best to comment this, someone with a bit more knowledge on HLFIR/FIR may be able to call me out on the details though.
|
Small ping for some reviewer attention if at all possible please! |
|
Sorry everyone, just the usual ping to ask for a little reviewer attention if that's at all possible, thank you all very much! :-) |
|
Small ping for some reviewer attention next week if possible please :-) |
TIFitis
approved these changes
Sep 24, 2025
kparzysz
approved these changes
Sep 25, 2025
…type array maps The main problem is that the previous intermediate map generation for allocatable members wasn't quite handling negative bounds acccesses correctly, it seems to require slightly more complicated access using shape_shift/dimension information. So this more closely mimics what Flang generates in other cases now. There is still a path for non-Box types to go down the old route for the moment, so it is possible we may still have issues with negative bounds in these cases. But, that's better in another PR if we come across it, instead of too much change in this one.
agozillon
force-pushed
the
neg-bounds-map-error
branch
from
c73d86a to
36a6c67
Compare
|
Thank you very much for the reviews everyone! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
The main problem is that the previous intermediate map generation for allocatable members wasn't quite handling negative bounds acccesses correctly, it seems to require slightly more complicated access using shape_shift/dimension information. So this more closely mimics what Flang generates in other cases now.
There is still a path for non-Box types to go down the old route for the moment, so it is possible we may still have issues with negative bounds in these cases. But, that's better in another PR if we come across it, instead of too much change in this one.