Skip to content

Commit

Permalink
[flang][CodeGen] add nsw to address calculations (#74709)
Browse files Browse the repository at this point in the history 
`nsw` is a flag for LLVM arithmetic operations meaning "no signed wrap".
If this keyword is present, the result of the operation is a poison
value if overflow occurs. Adding this keyword permits LLVM to re-order
integer arithmetic more aggressively.

In

https://discourse.llvm.org/t/rfc-changes-to-fircg-xarray-coor-codegen-to-allow-better-hoisting/75257/16
@vzakhari observed that adding nsw is useful to enable hoisting of
address calculations after some loops (or is at least a step in that
direction).

Classic flang also adds nsw to address calculations.
  • Loading branch information
@tblah
tblah committed Dec 8, 2023
1 parent 633fe60 commit bdacd56
Show file tree
Hide file tree
Showing 6 changed files with 115 additions and 104 deletions.
41 changes: 26 additions & 15 deletions flang/lib/Optimizer/CodeGen/CodeGen.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2387,6 +2387,9 @@ struct XArrayCoorOpConversion
const bool baseIsBoxed = coor.getMemref().getType().isa<fir::BaseBoxType>();
TypePair baseBoxTyPair =
baseIsBoxed ? getBoxTypePair(coor.getMemref().getType()) : TypePair{};
mlir::LLVM::IntegerOverflowFlagsAttr nsw =
mlir::LLVM::IntegerOverflowFlagsAttr::get(
rewriter.getContext(), mlir::LLVM::IntegerOverflowFlags::nsw);

// For each dimension of the array, generate the offset calculation.
for (unsigned i = 0; i < rank; ++i, ++indexOffset, ++shapeOffset,
Expand All @@ -2407,32 +2410,37 @@ struct XArrayCoorOpConversion
if (normalSlice)
step = integerCast(loc, rewriter, idxTy, operands[sliceOffset + 2]);
}
auto idx = rewriter.create<mlir::LLVM::SubOp>(loc, idxTy, index, lb);
auto idx = rewriter.create<mlir::LLVM::SubOp>(loc, idxTy, index, lb, nsw);
mlir::Value diff =
rewriter.create<mlir::LLVM::MulOp>(loc, idxTy, idx, step);
rewriter.create<mlir::LLVM::MulOp>(loc, idxTy, idx, step, nsw);
if (normalSlice) {
mlir::Value sliceLb =
integerCast(loc, rewriter, idxTy, operands[sliceOffset]);
auto adj = rewriter.create<mlir::LLVM::SubOp>(loc, idxTy, sliceLb, lb);
diff = rewriter.create<mlir::LLVM::AddOp>(loc, idxTy, diff, adj);
auto adj =
rewriter.create<mlir::LLVM::SubOp>(loc, idxTy, sliceLb, lb, nsw);
diff = rewriter.create<mlir::LLVM::AddOp>(loc, idxTy, diff, adj, nsw);
}
// Update the offset given the stride and the zero based index `diff`
// that was just computed.
if (baseIsBoxed) {
// Use stride in bytes from the descriptor.
mlir::Value stride =
getStrideFromBox(loc, baseBoxTyPair, operands[0], i, rewriter);
auto sc = rewriter.create<mlir::LLVM::MulOp>(loc, idxTy, diff, stride);
offset = rewriter.create<mlir::LLVM::AddOp>(loc, idxTy, sc, offset);
auto sc =
rewriter.create<mlir::LLVM::MulOp>(loc, idxTy, diff, stride, nsw);
offset =
rewriter.create<mlir::LLVM::AddOp>(loc, idxTy, sc, offset, nsw);
} else {
// Use stride computed at last iteration.
auto sc = rewriter.create<mlir::LLVM::MulOp>(loc, idxTy, diff, prevExt);
offset = rewriter.create<mlir::LLVM::AddOp>(loc, idxTy, sc, offset);
auto sc =
rewriter.create<mlir::LLVM::MulOp>(loc, idxTy, diff, prevExt, nsw);
offset =
rewriter.create<mlir::LLVM::AddOp>(loc, idxTy, sc, offset, nsw);
// Compute next stride assuming contiguity of the base array
// (in element number).
auto nextExt = integerCast(loc, rewriter, idxTy, operands[shapeOffset]);
prevExt =
rewriter.create<mlir::LLVM::MulOp>(loc, idxTy, prevExt, nextExt);
prevExt = rewriter.create<mlir::LLVM::MulOp>(loc, idxTy, prevExt,
nextExt, nsw);
}
}

Expand Down Expand Up @@ -2491,8 +2499,8 @@ struct XArrayCoorOpConversion
assert(coor.getLenParams().size() == 1);
auto length = integerCast(loc, rewriter, idxTy,
operands[coor.lenParamsOffset()]);
offset =
rewriter.create<mlir::LLVM::MulOp>(loc, idxTy, offset, length);
offset = rewriter.create<mlir::LLVM::MulOp>(loc, idxTy, offset,
length, nsw);
} else {
TODO(loc, "compute size of derived type with type parameters");
}
Expand Down Expand Up @@ -2665,6 +2673,9 @@ struct CoordinateOpConversion
auto cpnTy = fir::dyn_cast_ptrOrBoxEleTy(boxObjTy);
mlir::Type llvmPtrTy = ::getLlvmPtrType(coor.getContext());
mlir::Type byteTy = ::getI8Type(coor.getContext());
mlir::LLVM::IntegerOverflowFlagsAttr nsw =
mlir::LLVM::IntegerOverflowFlagsAttr::get(
rewriter.getContext(), mlir::LLVM::IntegerOverflowFlags::nsw);

for (unsigned i = 1, last = operands.size(); i < last; ++i) {
if (auto arrTy = cpnTy.dyn_cast<fir::SequenceType>()) {
Expand All @@ -2680,9 +2691,9 @@ struct CoordinateOpConversion
index < lastIndex; ++index) {
mlir::Value stride = getStrideFromBox(loc, boxTyPair, operands[0],
index - i, rewriter);
auto sc = rewriter.create<mlir::LLVM::MulOp>(loc, idxTy,
operands[index], stride);
off = rewriter.create<mlir::LLVM::AddOp>(loc, idxTy, sc, off);
auto sc = rewriter.create<mlir::LLVM::MulOp>(
loc, idxTy, operands[index], stride, nsw);
off = rewriter.create<mlir::LLVM::AddOp>(loc, idxTy, sc, off, nsw);
}
resultAddr = rewriter.create<mlir::LLVM::GEPOp>(
loc, llvmPtrTy, byteTy, resultAddr,
Expand Down
12 changes: 6 additions & 6 deletions flang/test/Fir/array-coor.fir
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,12 @@ func.func @array_coor_box_value(%29 : !fir.box<!fir.array<2xf64>>,
}

// CHECK-LABEL: define double @array_coor_box_value
// CHECK: %[[t3:.*]] = sub i64 %{{.*}}, 1
// CHECK: %[[t4:.*]] = mul i64 %[[t3]], 1
// CHECK: %[[t3:.*]] = sub nsw i64 %{{.*}}, 1
// CHECK: %[[t4:.*]] = mul nsw i64 %[[t3]], 1
// CHECK: %[[t5:.*]] = getelementptr { ptr, i64, i32, i8, i8, i8, i8, [1 x [3 x i64]] }, ptr %{{.*}}, i32 0, i32 7, i32 0, i32 2
// CHECK: %[[t6:.*]] = load i64, ptr %[[t5]]
// CHECK: %[[t7:.*]] = mul i64 %[[t4]], %[[t6]]
// CHECK: %[[t8:.*]] = add i64 %[[t7]], 0
// CHECK: %[[t7:.*]] = mul nsw i64 %[[t4]], %[[t6]]
// CHECK: %[[t8:.*]] = add nsw i64 %[[t7]], 0
// CHECK: %[[t9:.*]] = getelementptr { ptr, i64, i32, i8, i8, i8, i8, [1 x [3 x i64]] }, ptr %{{.*}}, i32 0, i32 0
// CHECK: %[[t10:.*]] = load ptr, ptr %[[t9]]
// CHECK: %[[t11:.*]] = getelementptr i8, ptr %[[t10]], i64 %[[t8]]
Expand All @@ -36,8 +36,8 @@ func.func private @take_int(%arg0: !fir.ref<i32>) -> ()
// CHECK-SAME: ptr %[[VAL_0:.*]])
// CHECK: %[[VAL_1:.*]] = getelementptr { ptr, i64, i32, i8, i8, i8, i8, [1 x [3 x i64]], ptr, [1 x i64] }, ptr %[[VAL_0]], i32 0, i32 7, i32 0, i32 2
// CHECK: %[[VAL_2:.*]] = load i64, ptr %[[VAL_1]]
// CHECK: %[[VAL_3:.*]] = mul i64 1, %[[VAL_2]]
// CHECK: %[[VAL_4:.*]] = add i64 %[[VAL_3]], 0
// CHECK: %[[VAL_3:.*]] = mul nsw i64 1, %[[VAL_2]]
// CHECK: %[[VAL_4:.*]] = add nsw i64 %[[VAL_3]], 0
// CHECK: %[[VAL_5:.*]] = getelementptr { ptr, i64, i32, i8, i8, i8, i8, [1 x [3 x i64]], ptr, [1 x i64] }, ptr %[[VAL_0]], i32 0, i32 0
// CHECK: %[[VAL_6:.*]] = load ptr, ptr %[[VAL_5]]
// CHECK: %[[VAL_7:.*]] = getelementptr i8, ptr %[[VAL_6]], i64 %[[VAL_4]]
Expand Down
8 changes: 4 additions & 4 deletions flang/test/Fir/arrexp.fir
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,8 @@ func.func @f5(%arg0: !fir.box<!fir.array<?xf32>>, %arg1: !fir.box<!fir.array<?xf
%4 = fir.do_loop %arg3 = %c0 to %1 step %c1 iter_args(%arg4 = %2) -> (!fir.array<?xf32>) {
// CHECK: %[[B_STRIDE_GEP:.*]] = getelementptr {{.*}}, ptr %[[B]], i32 0, i32 7, i32 0, i32 2
// CHECK: %[[B_STRIDE:.*]] = load i64, ptr %[[B_STRIDE_GEP]]
// CHECK: %[[B_DIM_OFFSET:.*]] = mul i64 %{{.*}}, %[[B_STRIDE]]
// CHECK: %[[B_OFFSET:.*]] = add i64 %[[B_DIM_OFFSET]], 0
// CHECK: %[[B_DIM_OFFSET:.*]] = mul nsw i64 %{{.*}}, %[[B_STRIDE]]
// CHECK: %[[B_OFFSET:.*]] = add nsw i64 %[[B_DIM_OFFSET]], 0
// CHECK: %[[B_BASE_GEP:.*]] = getelementptr {{.*}}, ptr %{{.*}}, i32 0, i32 0
// CHECK: %[[B_BASE:.*]] = load ptr, ptr %[[B_BASE_GEP]]
// CHECK: %[[B_VOID_ADDR:.*]] = getelementptr i8, ptr %[[B_BASE]], i64 %[[B_OFFSET]]
Expand Down Expand Up @@ -172,7 +172,7 @@ func.func @f7(%arg0: !fir.ref<f32>, %arg1: !fir.box<!fir.array<?xf32>>) {
%0 = fir.shift %c4 : (index) -> !fir.shift<1>
// CHECK: %[[STRIDE_GEP:.*]] = getelementptr {{.*}}, ptr %[[Y]], i32 0, i32 7, i32 0, i32 2
// CHECK: %[[STRIDE:.*]] = load i64, ptr %[[STRIDE_GEP]]
// CHECK: mul i64 96, %[[STRIDE]]
// CHECK: mul nsw i64 96, %[[STRIDE]]
%1 = fir.array_coor %arg1(%0) %c100 : (!fir.box<!fir.array<?xf32>>, !fir.shift<1>, index) -> !fir.ref<f32>
%2 = fir.load %1 : !fir.ref<f32>
fir.store %2 to %arg0 : !fir.ref<f32>
Expand Down Expand Up @@ -202,7 +202,7 @@ func.func @f8(%a : !fir.ref<!fir.array<2x2x!fir.type<t{i:i32}>>>, %i : i32) {
func.func @f9(%i: i32, %e : i64, %j: i64, %c: !fir.ref<!fir.array<?x?x!fir.char<1,?>>>) -> !fir.ref<!fir.char<1,?>> {
%s = fir.shape %e, %e : (i64, i64) -> !fir.shape<2>
// CHECK: %[[CAST:.*]] = sext i32 %[[I]] to i64
// CHECK: %[[OFFSET:.*]] = mul i64 %{{.*}}, %[[CAST]]
// CHECK: %[[OFFSET:.*]] = mul nsw i64 %{{.*}}, %[[CAST]]
// CHECK: getelementptr i8, ptr %[[C]], i64 %[[OFFSET]]
%a = fir.array_coor %c(%s) %j, %j typeparams %i : (!fir.ref<!fir.array<?x?x!fir.char<1,?>>>, !fir.shape<2>, i64, i64, i32) -> !fir.ref<!fir.char<1,?>>
return %a : !fir.ref<!fir.char<1,?>>
Expand Down

3 comments on commit bdacd56

@stefanp-ibm
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tblah
It looks like this patch broke the flang bot for PowerPC.
https://lab.llvm.org/buildbot/#/builders/21

Please fix the issue or revert the patch.

If you have questions about how to reproduce this issue please let me know.

@tblah
Copy link
Contributor Author

@tblah tblah commented on bdacd56 Dec 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#74826

@stefanp-ibm
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#74826

Thank you!

Please sign in to comment.