Skip to content

Commit

Permalink
[flang] Generate valid IR on GOTO DO body (#66084)
Browse files Browse the repository at this point in the history 
Flang was generating invalid IR when there was a GOTO to the body
of a DO loop. This happened because the value of step, computed at
the beginning of the loop, was being reused at the end of the loop,
that, for unstructured loops, is in another basic block. Because of
this, a GOTO could skip the beginning of the loop, that defined
step, and yet try to use it at the end of the loop, which is
invalid.

Instead of reusing the step value, it can be recomputed if it is a
constant, or stored and loaded to/from a temporary variable, for
non-constant step expressions.

Note that, while this change prevents the generation of invalid IR
on the presence of jumps to DO loop bodies, what happens if the
program reaches the end of a DO loop without ever passing through
its beginning is undefined behavior, as some control variables,
such as trip, will be uninitialized. It doesn't seem worth the
effort and overhead to ensure this legacy extension will behave
correctly in this case. This is consistent with at least gfortran,
that doesn't behave correctly if step is not equal to one.

Fixes: #65036
  • Loading branch information
@luporl
luporl authored Sep 21, 2023
1 parent 7e74446 commit 3dbb055
Show file tree
Hide file tree
Showing 7 changed files with 309 additions and 40 deletions.
70 changes: 44 additions & 26 deletions flang/lib/Lower/Bridge.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,14 +112,14 @@ struct IncrementLoopInfo {
llvm::SmallVector<const Fortran::semantics::Symbol *> localInitSymList;
llvm::SmallVector<const Fortran::semantics::Symbol *> sharedSymList;
mlir::Value loopVariable = nullptr;
mlir::Value stepValue = nullptr; // possible uses in multiple blocks

// Data members for structured loops.
fir::DoLoopOp doLoop = nullptr;

// Data members for unstructured loops.
bool hasRealControl = false;
mlir::Value tripVariable = nullptr;
mlir::Value stepVariable = nullptr;
mlir::Block *headerBlock = nullptr; // loop entry and test block
mlir::Block *maskBlock = nullptr; // concurrent loop mask block
mlir::Block *bodyBlock = nullptr; // first loop body block
Expand Down Expand Up @@ -1738,29 +1738,45 @@ class FirConverter : public Fortran::lower::AbstractConverter {
genFIR(endDoEval, unstructuredContext);
}

/// Generate FIR to evaluate loop control values (lower, upper and step).
mlir::Value genControlValue(const Fortran::lower::SomeExpr *expr,
const IncrementLoopInfo &info,
bool *isConst = nullptr) {
mlir::Location loc = toLocation();
mlir::Type controlType = info.isStructured() ? builder->getIndexType()
: info.getLoopVariableType();
Fortran::lower::StatementContext stmtCtx;
if (expr) {
if (isConst)
*isConst = Fortran::evaluate::IsConstantExpr(*expr);
return builder->createConvert(loc, controlType,
createFIRExpr(loc, expr, stmtCtx));
}

if (isConst)
*isConst = true;
if (info.hasRealControl)
return builder->createRealConstant(loc, controlType, 1u);
return builder->createIntegerConstant(loc, controlType, 1); // step
}

/// Generate FIR to begin a structured or unstructured increment loop nest.
void genFIRIncrementLoopBegin(IncrementLoopNestInfo &incrementLoopNestInfo) {
assert(!incrementLoopNestInfo.empty() && "empty loop nest");
mlir::Location loc = toLocation();
auto genControlValue = [&](const Fortran::lower::SomeExpr *expr,
const IncrementLoopInfo &info) {
mlir::Type controlType = info.isStructured() ? builder->getIndexType()
: info.getLoopVariableType();
Fortran::lower::StatementContext stmtCtx;
if (expr)
return builder->createConvert(loc, controlType,
createFIRExpr(loc, expr, stmtCtx));

if (info.hasRealControl)
return builder->createRealConstant(loc, controlType, 1u);
return builder->createIntegerConstant(loc, controlType, 1); // step
};
for (IncrementLoopInfo &info : incrementLoopNestInfo) {
info.loopVariable =
genLoopVariableAddress(loc, info.loopVariableSym, info.isUnordered);
mlir::Value lowerValue = genControlValue(info.lowerExpr, info);
mlir::Value upperValue = genControlValue(info.upperExpr, info);
info.stepValue = genControlValue(info.stepExpr, info);
bool isConst = true;
mlir::Value stepValue = genControlValue(
info.stepExpr, info, info.isStructured() ? nullptr : &isConst);
// Use a temp variable for unstructured loops with non-const step.
if (!isConst) {
info.stepVariable = builder->createTemporary(loc, stepValue.getType());
builder->create<fir::StoreOp>(loc, stepValue, info.stepVariable);
}

// Structured loop - generate fir.do_loop.
if (info.isStructured()) {
Expand All @@ -1769,14 +1785,14 @@ class FirConverter : public Fortran::lower::AbstractConverter {
if (info.isUnordered) {
// The loop variable value is explicitly updated.
info.doLoop = builder->create<fir::DoLoopOp>(
loc, lowerValue, upperValue, info.stepValue, /*unordered=*/true);
loc, lowerValue, upperValue, stepValue, /*unordered=*/true);
builder->setInsertionPointToStart(info.doLoop.getBody());
loopValue = builder->createConvert(loc, loopVarType,
info.doLoop.getInductionVar());
} else {
// The loop variable is a doLoop op argument.
info.doLoop = builder->create<fir::DoLoopOp>(
loc, lowerValue, upperValue, info.stepValue, /*unordered=*/false,
loc, lowerValue, upperValue, stepValue, /*unordered=*/false,
/*finalCountValue=*/true,
builder->createConvert(loc, loopVarType, lowerValue));
builder->setInsertionPointToStart(info.doLoop.getBody());
Expand Down Expand Up @@ -1805,18 +1821,17 @@ class FirConverter : public Fortran::lower::AbstractConverter {
auto diff1 =
builder->create<mlir::arith::SubFOp>(loc, upperValue, lowerValue);
auto diff2 =
builder->create<mlir::arith::AddFOp>(loc, diff1, info.stepValue);
tripCount =
builder->create<mlir::arith::DivFOp>(loc, diff2, info.stepValue);
builder->create<mlir::arith::AddFOp>(loc, diff1, stepValue);
tripCount = builder->create<mlir::arith::DivFOp>(loc, diff2, stepValue);
tripCount =
builder->createConvert(loc, builder->getIndexType(), tripCount);
} else {
auto diff1 =
builder->create<mlir::arith::SubIOp>(loc, upperValue, lowerValue);
auto diff2 =
builder->create<mlir::arith::AddIOp>(loc, diff1, info.stepValue);
builder->create<mlir::arith::AddIOp>(loc, diff1, stepValue);
tripCount =
builder->create<mlir::arith::DivSIOp>(loc, diff2, info.stepValue);
builder->create<mlir::arith::DivSIOp>(loc, diff2, stepValue);
}
if (forceLoopToExecuteOnce) { // minimum tripCount is 1
mlir::Value one =
Expand Down Expand Up @@ -1904,12 +1919,15 @@ class FirConverter : public Fortran::lower::AbstractConverter {
tripCount = builder->create<mlir::arith::SubIOp>(loc, tripCount, one);
builder->create<fir::StoreOp>(loc, tripCount, info.tripVariable);
mlir::Value value = builder->create<fir::LoadOp>(loc, info.loopVariable);
mlir::Value step;
if (info.stepVariable)
step = builder->create<fir::LoadOp>(loc, info.stepVariable);
else
step = genControlValue(info.stepExpr, info);
if (info.hasRealControl)
value =
builder->create<mlir::arith::AddFOp>(loc, value, info.stepValue);
value = builder->create<mlir::arith::AddFOp>(loc, value, step);
else
value =
builder->create<mlir::arith::AddIOp>(loc, value, info.stepValue);
value = builder->create<mlir::arith::AddIOp>(loc, value, step);
builder->create<fir::StoreOp>(loc, value, info.loopVariable);

genBranch(info.headerBlock);
Expand Down
114 changes: 114 additions & 0 deletions flang/test/Lower/HLFIR/goto-do-body.f90
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
! RUN: bbc --hlfir -o - %s | FileCheck %s

! Test jumping to the body of a do loop.
subroutine sub1()
! CHECK-LABEL: func @_QPsub1() {
implicit none
integer :: i
external foo
! CHECK: %[[C2:.*]] = arith.constant 2 : i32
! CHECK: %[[C0:.*]] = arith.constant 0 : i32
! CHECK: %[[C1:.*]] = arith.constant 1 : i32
! CHECK: %[[TRIP:.*]] = fir.alloca i32
! CHECK: %[[I_REF:.*]] = fir.alloca i32 {bindc_name = "i", {{.*}}}
! CHECK: %[[I:.*]]:2 = hlfir.declare %[[I_REF]] {uniq_name = "_QFsub1Ei"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)

do i = 1, 3
if (i .eq. 2) goto 70
! CHECK: %[[TMP1:.*]] = fir.load %[[I]]#0 : !fir.ref<i32>
! CHECK: %[[COND:.*]] = arith.cmpi eq, %[[TMP1]], %[[C2]] : i32
! CHECK: cf.cond_br %[[COND]], ^[[BODY:.*]], ^{{.*}}

end do

call foo
! CHECK: fir.call @_QPfoo()

do i = 1, 2
! CHECK: fir.store %[[C2]] to %[[TRIP]] : !fir.ref<i32>
! CHECK: fir.store %[[C1]] to %[[I]]#1 : !fir.ref<i32>
! CHECK: cf.br ^[[HEADER:.*]]
! CHECK: ^[[HEADER]]:
! CHECK: %[[TMP2:.*]] = fir.load %[[TRIP]] : !fir.ref<i32>
! CHECK: %[[TMP3:.*]] = arith.cmpi sgt, %[[TMP2]], %[[C0]] : i32
! CHECK: cf.cond_br %[[TMP3]], ^[[BODY]], ^[[EXIT:.*]]

70 call foo
! CHECK: ^[[BODY]]:
! CHECK: fir.call @_QPfoo()
! CHECK: %[[TMP4:.*]] = fir.load %[[TRIP]] : !fir.ref<i32>
! CHECK: %[[TMP5:.*]] = arith.subi %[[TMP4]], %[[C1]] : i32
! CHECK: fir.store %[[TMP5]] to %[[TRIP]] : !fir.ref<i32>
! CHECK: %[[TMP6:.*]] = fir.load %[[I]]#1 : !fir.ref<i32>
! CHECK: %[[TMP7:.*]] = arith.addi %[[TMP6]], %[[C1]] : i32
! CHECK: fir.store %[[TMP7]] to %[[I]]#1 : !fir.ref<i32>
! CHECK: cf.br ^[[HEADER]]
end do
end subroutine
! CHECK: ^[[EXIT]]:
! CHECK: return
! CHECK: }

! Test jumping to the body of a do loop with a step expression.
subroutine sub2()
! CHECK-LABEL: func @_QPsub2() {
implicit none
integer :: i, j
external foo
! CHECK: %[[C_7:.*]] = arith.constant -7 : i32
! CHECK: %[[C8:.*]] = arith.constant 8 : i32
! CHECK: %[[C2:.*]] = arith.constant 2 : i32
! CHECK: %[[C0:.*]] = arith.constant 0 : i32
! CHECK: %[[C3:.*]] = arith.constant 3 : i32
! CHECK: %[[C1:.*]] = arith.constant 1 : i32
! CHECK: %[[TRIP:.*]] = fir.alloca i32
! CHECK: %[[I_REF:.*]] = fir.alloca i32 {bindc_name = "i", {{.*}}}
! CHECK: %[[I:.*]]:2 = hlfir.declare %[[I_REF]] {uniq_name = "_QFsub2Ei"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
! CHECK: %[[J_REF:.*]] = fir.alloca i32 {bindc_name = "j", {{.*}}}
! CHECK: %[[J:.*]]:2 = hlfir.declare %[[J_REF]] {uniq_name = "_QFsub2Ej"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)

do i = 1, 3
if (i .eq. 2) goto 70
! CHECK: %[[TMP1:.*]] = fir.load %[[I]]#0 : !fir.ref<i32>
! CHECK: %[[COND:.*]] = arith.cmpi eq, %[[TMP1]], %[[C2]] : i32
! CHECK: cf.cond_br %[[COND]], ^[[BODY:.*]], ^{{.*}}

end do

call foo
! CHECK: fir.call @_QPfoo()

j = 3
! CHECK: hlfir.assign %[[C3]] to %[[J]]#0 : i32, !fir.ref<i32>

do i = 1, 2, 3 * j - 8
! CHECK: %[[TMP2:.*]] = fir.load %[[J]]#0 : !fir.ref<i32>
! CHECK: %[[TMP3:.*]] = arith.muli %[[TMP2]], %[[C3]] : i32
! CHECK: %[[STEP:.*]] = arith.subi %[[TMP3]], %[[C8]] : i32
! CHECK: fir.store %[[STEP]] to %[[STEP_VAR:.*]] : !fir.ref<i32>
! CHECK: %[[TMP4:.*]] = arith.addi %[[TMP3]], %[[C_7]] : i32
! CHECK: %[[TMP5:.*]] = arith.divsi %[[TMP4]], %[[STEP]] : i32
! CHECK: fir.store %[[TMP5]] to %[[TRIP]] : !fir.ref<i32>
! CHECK: fir.store %[[C1]] to %[[I]]#1 : !fir.ref<i32>
! CHECK: cf.br ^[[HEADER:.*]]
! CHECK: ^[[HEADER]]:
! CHECK: %[[TMP6:.*]] = fir.load %[[TRIP]] : !fir.ref<i32>
! CHECK: %[[TMP7:.*]] = arith.cmpi sgt, %[[TMP6]], %[[C0]] : i32
! CHECK: cf.cond_br %[[TMP7]], ^[[BODY]], ^[[EXIT:.*]]

70 call foo
! CHECK: ^[[BODY]]:
! CHECK: fir.call @_QPfoo()
! CHECK: %[[TMP8:.*]] = fir.load %[[TRIP]] : !fir.ref<i32>
! CHECK: %[[TMP9:.*]] = arith.subi %[[TMP8]], %[[C1]] : i32
! CHECK: fir.store %[[TMP9]] to %[[TRIP]] : !fir.ref<i32>
! CHECK: %[[TMP10:.*]] = fir.load %[[I]]#1 : !fir.ref<i32>
! CHECK: %[[STEP_VAL:.*]] = fir.load %[[STEP_VAR]] : !fir.ref<i32>
! CHECK: %[[TMP11:.*]] = arith.addi %[[TMP10]], %[[STEP_VAL]] : i32
! CHECK: fir.store %[[TMP11]] to %[[I]]#1 : !fir.ref<i32>
! CHECK: cf.br ^[[HEADER]]
end do
end subroutine
! CHECK: ^[[EXIT]]:
! CHECK: return
! CHECK: }
4 changes: 3 additions & 1 deletion flang/test/Lower/do_loop.f90
View file
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,7 @@ subroutine loop_with_real_control(s,e,st)
! CHECK-DAG: %[[S:.*]] = fir.load %[[S_REF]] : !fir.ref<f32>
! CHECK-DAG: %[[E:.*]] = fir.load %[[E_REF]] : !fir.ref<f32>
! CHECK-DAG: %[[ST:.*]] = fir.load %[[ST_REF]] : !fir.ref<f32>
! CHECK: fir.store %[[ST]] to %[[ST_VAR:.*]] : !fir.ref<f32>
real :: x, s, e, st

! CHECK: %[[DIFF:.*]] = arith.subf %[[E]], %[[S]] {{.*}}: f32
Expand All @@ -267,7 +268,8 @@ subroutine loop_with_real_control(s,e,st)
! CHECK: %[[INC:.*]] = arith.subi %[[INDEX2]], %[[C1]] : index
! CHECK: fir.store %[[INC]] to %[[INDEX_REF]] : !fir.ref<index>
! CHECK: %[[X2:.*]] = fir.load %[[X_REF]] : !fir.ref<f32>
! CHECK: %[[XINC:.*]] = arith.addf %[[X2]], %[[ST]] {{.*}}: f32
! CHECK: %[[ST_VAL:.*]] = fir.load %[[ST_VAR]] : !fir.ref<f32>
! CHECK: %[[XINC:.*]] = arith.addf %[[X2]], %[[ST_VAL]] {{.*}}: f32
! CHECK: fir.store %[[XINC]] to %[[X_REF]] : !fir.ref<f32>
! CHECK: br ^[[HDR]]
end do
Expand Down
18 changes: 12 additions & 6 deletions flang/test/Lower/do_loop_unstructured.f90
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ subroutine simple_unstructured()
! CHECK: %[[TRIP_VAR_NEXT:.*]] = arith.subi %[[TRIP_VAR]], %[[ONE_1]] : i32
! CHECK: fir.store %[[TRIP_VAR_NEXT]] to %[[TRIP_VAR_REF]] : !fir.ref<i32>
! CHECK: %[[LOOP_VAR:.*]] = fir.load %[[LOOP_VAR_REF]] : !fir.ref<i32>
! CHECK: %[[LOOP_VAR_NEXT:.*]] = arith.addi %[[LOOP_VAR]], %[[STEP_ONE]] : i32
! CHECK: %[[STEP_ONE_2:.*]] = arith.constant 1 : i32
! CHECK: %[[LOOP_VAR_NEXT:.*]] = arith.addi %[[LOOP_VAR]], %[[STEP_ONE_2]] : i32
! CHECK: fir.store %[[LOOP_VAR_NEXT]] to %[[LOOP_VAR_REF]] : !fir.ref<i32>
! CHECK: cf.br ^[[HEADER]]
! CHECK: ^[[EXIT]]:
Expand Down Expand Up @@ -75,7 +76,8 @@ subroutine simple_unstructured_with_step()
! CHECK: %[[TRIP_VAR_NEXT:.*]] = arith.subi %[[TRIP_VAR]], %[[ONE_1]] : i32
! CHECK: fir.store %[[TRIP_VAR_NEXT]] to %[[TRIP_VAR_REF]] : !fir.ref<i32>
! CHECK: %[[LOOP_VAR:.*]] = fir.load %[[LOOP_VAR_REF]] : !fir.ref<i32>
! CHECK: %[[LOOP_VAR_NEXT:.*]] = arith.addi %[[LOOP_VAR]], %[[STEP]] : i32
! CHECK: %[[STEP_2:.*]] = arith.constant 2 : i32
! CHECK: %[[LOOP_VAR_NEXT:.*]] = arith.addi %[[LOOP_VAR]], %[[STEP_2]] : i32
! CHECK: fir.store %[[LOOP_VAR_NEXT]] to %[[LOOP_VAR_REF]] : !fir.ref<i32>
! CHECK: cf.br ^[[HEADER]]
! CHECK: ^[[EXIT]]:
Expand Down Expand Up @@ -151,7 +153,8 @@ subroutine nested_unstructured()
! CHECK: %[[TRIP_VAR_K_NEXT:.*]] = arith.subi %[[TRIP_VAR_K]], %[[ONE_1]] : i32
! CHECK: fir.store %[[TRIP_VAR_K_NEXT]] to %[[TRIP_VAR_K_REF]] : !fir.ref<i32>
! CHECK: %[[LOOP_VAR_K:.*]] = fir.load %[[LOOP_VAR_K_REF]] : !fir.ref<i32>
! CHECK: %[[LOOP_VAR_K_NEXT:.*]] = arith.addi %[[LOOP_VAR_K]], %[[K_STEP]] : i32
! CHECK: %[[K_STEP_2:.*]] = arith.constant 1 : i32
! CHECK: %[[LOOP_VAR_K_NEXT:.*]] = arith.addi %[[LOOP_VAR_K]], %[[K_STEP_2]] : i32
! CHECK: fir.store %[[LOOP_VAR_K_NEXT]] to %[[LOOP_VAR_K_REF]] : !fir.ref<i32>
! CHECK: cf.br ^[[HEADER_K]]
! CHECK: ^[[EXIT_K]]:
Expand All @@ -160,7 +163,8 @@ subroutine nested_unstructured()
! CHECK: %[[TRIP_VAR_J_NEXT:.*]] = arith.subi %[[TRIP_VAR_J]], %[[ONE_1]] : i32
! CHECK: fir.store %[[TRIP_VAR_J_NEXT]] to %[[TRIP_VAR_J_REF]] : !fir.ref<i32>
! CHECK: %[[LOOP_VAR_J:.*]] = fir.load %[[LOOP_VAR_J_REF]] : !fir.ref<i32>
! CHECK: %[[LOOP_VAR_J_NEXT:.*]] = arith.addi %[[LOOP_VAR_J]], %[[J_STEP]] : i32
! CHECK: %[[J_STEP_2:.*]] = arith.constant 1 : i32
! CHECK: %[[LOOP_VAR_J_NEXT:.*]] = arith.addi %[[LOOP_VAR_J]], %[[J_STEP_2]] : i32
! CHECK: fir.store %[[LOOP_VAR_J_NEXT]] to %[[LOOP_VAR_J_REF]] : !fir.ref<i32>
! CHECK: cf.br ^[[HEADER_J]]
! CHECK: ^[[EXIT_J]]:
Expand All @@ -169,7 +173,8 @@ subroutine nested_unstructured()
! CHECK: %[[TRIP_VAR_I_NEXT:.*]] = arith.subi %[[TRIP_VAR_I]], %[[ONE_1]] : i32
! CHECK: fir.store %[[TRIP_VAR_I_NEXT]] to %[[TRIP_VAR_I_REF]] : !fir.ref<i32>
! CHECK: %[[LOOP_VAR_I:.*]] = fir.load %[[LOOP_VAR_I_REF]] : !fir.ref<i32>
! CHECK: %[[LOOP_VAR_I_NEXT:.*]] = arith.addi %[[LOOP_VAR_I]], %[[I_STEP]] : i32
! CHECK: %[[I_STEP_2:.*]] = arith.constant 1 : i32
! CHECK: %[[LOOP_VAR_I_NEXT:.*]] = arith.addi %[[LOOP_VAR_I]], %[[I_STEP_2]] : i32
! CHECK: fir.store %[[LOOP_VAR_I_NEXT]] to %[[LOOP_VAR_I_REF]] : !fir.ref<i32>
! CHECK: cf.br ^[[HEADER_I]]
! CHECK: ^[[EXIT_I]]:
Expand Down Expand Up @@ -215,7 +220,8 @@ subroutine nested_structured_in_unstructured()
! CHECK: %[[TRIP_VAR_I_NEXT:.*]] = arith.subi %[[TRIP_VAR_I]], %[[C1_3]] : i32
! CHECK: fir.store %[[TRIP_VAR_I_NEXT]] to %[[TRIP_VAR_I_REF]] : !fir.ref<i32>
! CHECK: %[[LOOP_VAR_I:.*]] = fir.load %[[LOOP_VAR_I_REF]] : !fir.ref<i32>
! CHECK: %[[LOOP_VAR_I_NEXT:.*]] = arith.addi %[[LOOP_VAR_I]], %c1_i32_0 : i32
! CHECK: %[[I_STEP_2:.*]] = arith.constant 1 : i32
! CHECK: %[[LOOP_VAR_I_NEXT:.*]] = arith.addi %[[LOOP_VAR_I]], %[[I_STEP_2]] : i32
! CHECK: fir.store %[[LOOP_VAR_I_NEXT]] to %[[LOOP_VAR_I_REF]] : !fir.ref<i32>
! CHECK: cf.br ^[[HEADER]]
! CHECK: ^[[EXIT]]:
Expand Down
Loading

0 comments on commit 3dbb055

Please sign in to comment.