Skip to content

Commit 716fe1c

Browse files
agozillonagozillon
authored
[Flang][OpenMP] Fix negative array indexing with allocatable derived type array maps (llvm#154193)
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.
1 parent 0b543e3 commit 716fe1c

File tree

3 files changed

+81
-22
lines changed

3 files changed

+81
-22
lines changed

‎flang/lib/Lower/OpenMP/Utils.cpp‎

Lines changed: 40 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -14,11 +14,12 @@
1414

1515
#include "ClauseFinder.h"
1616
#include "flang/Evaluate/fold.h"
17-
#include "flang/Lower/OpenMP/Clauses.h"
1817
#include <flang/Lower/AbstractConverter.h>
1918
#include <flang/Lower/ConvertType.h>
2019
#include <flang/Lower/DirectivesCommon.h>
20+
#include <flang/Lower/OpenMP/Clauses.h>
2121
#include <flang/Lower/PFTBuilder.h>
22+
#include <flang/Lower/Support/PrivateReductionUtils.h>
2223
#include <flang/Optimizer/Builder/FIRBuilder.h>
2324
#include <flang/Optimizer/Builder/Todo.h>
2425
#include <flang/Parser/openmp-utils.h>
@@ -180,16 +181,11 @@ static void generateArrayIndices(lower::AbstractConverter &converter,
180181
for (auto v : arr->subscript()) {
181182
if (std::holds_alternative<Triplet>(v.u))
182183
TODO(clauseLocation, "Triplet indexing in map clause is unsupported");
183-
184184
auto expr = std::get<Fortran::evaluate::IndirectSubscriptIntegerExpr>(v.u);
185185
mlir::Value subscript =
186186
fir::getBase(converter.genExprValue(toEvExpr(expr.value()), stmtCtx));
187-
mlir::Value one = firOpBuilder.createIntegerConstant(
188-
clauseLocation, firOpBuilder.getIndexType(), 1);
189-
subscript = firOpBuilder.createConvert(
190-
clauseLocation, firOpBuilder.getIndexType(), subscript);
191-
indices.push_back(mlir::arith::SubIOp::create(firOpBuilder, clauseLocation,
192-
subscript, one));
187+
indices.push_back(firOpBuilder.createConvert(
188+
clauseLocation, firOpBuilder.getIndexType(), subscript));
193189
}
194190
}
195191

@@ -322,10 +318,42 @@ mlir::Value createParentSymAndGenIntermediateMaps(
322318
subscriptIndices, objectList[i]);
323319
assert(!subscriptIndices.empty() &&
324320
"missing expected indices for map clause");
325-
curValue = fir::CoordinateOp::create(
326-
firOpBuilder, clauseLocation,
327-
firOpBuilder.getRefType(arrType.getEleTy()), curValue,
328-
subscriptIndices);
321+
if (auto boxTy = llvm::dyn_cast<fir::BaseBoxType>(curValue.getType())) {
322+
// To accommodate indexing into box types of all dimensions including
323+
// negative dimensions we have to take into consideration the lower
324+
// bounds and extents of the data (stored in the box) and convey it
325+
// to the ArrayCoorOp so that it can appropriately access the element
326+
// utilising the subscript we provide and the runtime sizes stored in
327+
// the Box. To do so we need to generate a ShapeShiftOp which combines
328+
// both the lb (ShiftOp) and extent (ShapeOp) of the Box, giving the
329+
// ArrayCoorOp the spatial information it needs to calculate the
330+
// underlying address.
331+
mlir::Value shapeShift = Fortran::lower::getShapeShift(
332+
firOpBuilder, clauseLocation, curValue);
333+
auto addrOp =
334+
fir::BoxAddrOp::create(firOpBuilder, clauseLocation, curValue);
335+
curValue = fir::ArrayCoorOp::create(
336+
firOpBuilder, clauseLocation,
337+
firOpBuilder.getRefType(arrType.getEleTy()), addrOp, shapeShift,
338+
/*slice=*/mlir::Value{}, subscriptIndices,
339+
/*typeparms=*/mlir::ValueRange{});
340+
} else {
341+
// We're required to negate by one in the non-Box case as I believe
342+
// we do not have the shape generated from the dimensions to help
343+
// adjust the indexing.
344+
// TODO/FIXME: This may need adjusted to support bounds of unusual
345+
// dimensions, if that's the case then it is likely best to fold this
346+
// branch into the above.
347+
mlir::Value one = firOpBuilder.createIntegerConstant(
348+
clauseLocation, firOpBuilder.getIndexType(), 1);
349+
for (auto &v : subscriptIndices)
350+
v = mlir::arith::SubIOp::create(firOpBuilder, clauseLocation, v,
351+
one);
352+
curValue = fir::CoordinateOp::create(
353+
firOpBuilder, clauseLocation,
354+
firOpBuilder.getRefType(arrType.getEleTy()), curValue,
355+
subscriptIndices);
356+
}
329357
}
330358
}
331359

@@ -415,7 +443,6 @@ mlir::Value createParentSymAndGenIntermediateMaps(
415443
currentIndicesIdx++;
416444
}
417445
}
418-
419446
return curValue;
420447
}
421448

‎flang/test/Integration/OpenMP/map-types-and-sizes.f90‎

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -785,15 +785,20 @@ end subroutine mapType_common_block_members
785785
!CHECK: %[[BOUNDS_CALC:.*]] = sub i64 %[[BOUNDS_LD_2]], 1
786786
!CHECK: %[[OFF_PTR_CALC_0:.*]] = sub i64 %[[BOUNDS_LD]], 1
787787
!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
788+
!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
789+
!CHECK: %[[LOAD_LB:.*]] = load i64, ptr %[[GEP_LB]], align 8
790+
!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
791+
!CHECK: %[[LOAD_UB:.*]] = load i64, ptr %[[GEP_UB]], align 8
788792
!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
789-
!CHECK: %[[LOAD_DESC_PTR:.*]] = load ptr, ptr %[[GEP_DESC_PTR]], align 8
790-
!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
791-
!CHECK: %[[SZ_CALC_2:.*]] = load i64, ptr %[[SZ_CALC_1]], align 8
792-
!CHECK: %[[SZ_CALC_3:.*]] = mul nsw i64 1, %[[SZ_CALC_2]]
793-
!CHECK: %[[SZ_CALC_4:.*]] = add nsw i64 %[[SZ_CALC_3]], 0
794-
!CHECK: %[[SZ_CALC_5:.*]] = getelementptr i8, ptr %[[LOAD_DESC_PTR]], i64 %[[SZ_CALC_4]]
795-
!CHECK: %[[SZ_CALC_6:.*]] = getelementptr %_QFmaptype_nested_derived_type_member_idxTvertexes, ptr %[[SZ_CALC_5]], i32 0, i32 2
796-
!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
793+
!CHECK: %[[SZ_CALC_1:.*]] = load ptr, ptr %[[GEP_DESC_PTR]], align 8
794+
!CHECK: %[[SZ_CALC_2:.*]] = sub nsw i64 2, %[[LOAD_LB]]
795+
!CHECK: %[[SZ_CALC_3:.*]] = mul nsw i64 %[[SZ_CALC_2]], 1
796+
!CHECK: %[[SZ_CALC_4:.*]] = mul nsw i64 %[[SZ_CALC_3]], 1
797+
!CHECK: %[[SZ_CALC_5:.*]] = add nsw i64 %[[SZ_CALC_4]], 0
798+
!CHECK: %[[SZ_CALC_6:.*]] = mul nsw i64 1, %[[LOAD_UB]]
799+
!CHECK: %[[SZ_CALC_7:.*]] = getelementptr %_QFmaptype_nested_derived_type_member_idxTvertexes, ptr %[[SZ_CALC_1]], i64 %[[SZ_CALC_5]]
800+
!CHECK: %[[SZ_CALC_8:.*]] = getelementptr %_QFmaptype_nested_derived_type_member_idxTvertexes, ptr %[[SZ_CALC_7]], i32 0, i32 2
801+
!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
797802
!CHECK: %[[OFF_PTR_CALC_1:.*]] = sub i64 %[[OFF_PTR_CALC_0]], 0
798803
!CHECK: %[[OFF_PTR_CALC_2:.*]] = add i64 %[[OFF_PTR_CALC_1]], 1
799804
!CHECK: %[[OFF_PTR_CALC_3:.*]] = mul i64 1, %[[OFF_PTR_CALC_2]]
@@ -838,7 +843,7 @@ end subroutine mapType_common_block_members
838843
!CHECK: %[[BASE_PTR_ARR:.*]] = getelementptr inbounds [7 x ptr], ptr %.offload_baseptrs, i32 0, i32 4
839844
!CHECK: store ptr %[[BASE_PTR_1]], ptr %[[BASE_PTR_ARR]], align 8
840845
!CHECK: %[[OFFLOAD_PTR_ARR:.*]] = getelementptr inbounds [7 x ptr], ptr %.offload_ptrs, i32 0, i32 4
841-
!CHECK: store ptr %[[SZ_CALC_6]], ptr %[[OFFLOAD_PTR_ARR]], align 8
846+
!CHECK: store ptr %[[SZ_CALC_8]], ptr %[[OFFLOAD_PTR_ARR]], align 8
842847
!CHECK: %[[BASE_PTR_ARR:.*]] = getelementptr inbounds [7 x ptr], ptr %.offload_baseptrs, i32 0, i32 5
843848
!CHECK: store ptr %[[BASE_PTR_1]], ptr %[[BASE_PTR_ARR]], align 8
844849
!CHECK: %[[OFFLOAD_PTR_ARR:.*]] = getelementptr inbounds [7 x ptr], ptr %.offload_ptrs, i32 0, i32 5

‎flang/test/Lower/OpenMP/map-neg-alloca-derived-type-array.f90‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
!RUN: %flang_fc1 -emit-hlfir -fopenmp %s -o - | FileCheck %s
2+
3+
subroutine map_negative_bounds_allocatable_dtype()
4+
type derived_type
5+
real(4), pointer :: data(:,:,:) => null()
6+
end type
7+
type(derived_type), allocatable :: dtype(:,:)
8+
9+
!$omp target map(tofrom: dtype(-1,1)%data)
10+
dtype(-1,1)%data(1,1,1) = 10
11+
!$omp end target
12+
end subroutine
13+
14+
! CHECK: %[[VAL_1:.*]] = arith.constant -1 : i64
15+
! CHECK: %[[VAL_2:.*]] = fir.convert %[[VAL_1]] : (i64) -> index
16+
! CHECK: %[[VAL_3:.*]] = arith.constant 1 : i64
17+
! CHECK: %[[VAL_4:.*]] = fir.convert %[[VAL_3]] : (i64) -> index
18+
! CHECK: %[[VAL_5:.*]] = arith.constant 0 : index
19+
! CHECK: %[[VAL_6:.*]]:3 = fir.box_dims %{{.*}}, %[[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)
20+
! CHECK: %[[VAL_7:.*]] = arith.constant 1 : index
21+
! CHECK: %[[VAL_8:.*]]:3 = fir.box_dims %{{.*}}, %[[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)
22+
! 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>
23+
! CHECK: %[[VAL_10:.*]] = fir.array_coor %{{.*}}(%[[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>>>}>>
24+
! CHECK: %[[VAL_11:.*]] = fir.coordinate_of %[[VAL_10]], 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>>>>
25+
! CHECK: %[[VAL_12:.*]] = fir.box_offset %[[VAL_11]] base_addr : (!fir.ref<!fir.box<!fir.ptr<!fir.array<?x?x?xf32>>>>) -> !fir.llvm_ptr<!fir.ref<!fir.array<?x?x?xf32>>>
26+
! CHECK: %[[VAL_13:.*]] = omp.map.info var_ptr(%[[VAL_11]] : !fir.ref<!fir.box<!fir.ptr<!fir.array<?x?x?xf32>>>>, f32) map_clauses(tofrom) capture(ByRef) var_ptr_ptr(%[[VAL_12]] : !fir.llvm_ptr<!fir.ref<!fir.array<?x?x?xf32>>>) bounds({{.*}}) -> !fir.llvm_ptr<!fir.ref<!fir.array<?x?x?xf32>>> {name = ""}
27+
! CHECK: %[[VAL_14:.*]] = omp.map.info var_ptr(%[[VAL_11]] : !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 = {{.*}}}

0 commit comments

Comments
 (0)