Skip to content

Commit 89ebea8

Browse files
NimishMishraNimishMishra
committed
[mlir][OpenMP] Fixed internal compiler error with atomic update operation verification
Fixes #61089 by updating the verification followed like translation from OpenMP+LLVM MLIR dialect to LLVM IR. Reviewed By: kiranchandramohan Differential Revision: https://reviews.llvm.org/D153217
1 parent 1a88292 commit 89ebea8

File tree

2 files changed

+45
-45
lines changed

2 files changed

+45
-45
lines changed

‎mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp‎

Lines changed: 42 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -1127,27 +1127,28 @@ convertOmpAtomicUpdate(omp::AtomicUpdateOp &opInst,
11271127

11281128
// Convert values and types.
11291129
auto &innerOpList = opInst.getRegion().front().getOperations();
1130-
if (innerOpList.size() != 2)
1131-
return opInst.emitError("exactly two operations are allowed inside an "
1132-
"atomic update region while lowering to LLVM IR");
1133-
1134-
Operation &innerUpdateOp = innerOpList.front();
1135-
1136-
if (innerUpdateOp.getNumOperands() != 2 ||
1137-
!llvm::is_contained(innerUpdateOp.getOperands(),
1138-
opInst.getRegion().getArgument(0)))
1139-
return opInst.emitError(
1140-
"the update operation inside the region must be a binary operation and "
1141-
"that update operation must have the region argument as an operand");
1142-
1143-
llvm::AtomicRMWInst::BinOp binop = convertBinOpToAtomic(innerUpdateOp);
1144-
1145-
bool isXBinopExpr =
1146-
innerUpdateOp.getNumOperands() > 0 &&
1147-
innerUpdateOp.getOperand(0) == opInst.getRegion().getArgument(0);
1130+
bool isRegionArgUsed{false}, isXBinopExpr{false};
1131+
llvm::AtomicRMWInst::BinOp binop;
1132+
mlir::Value mlirExpr;
1133+
// Find the binary update operation that uses the region argument
1134+
// and get the expression to update
1135+
for (Operation &innerOp : innerOpList) {
1136+
if (innerOp.getNumOperands() == 2) {
1137+
binop = convertBinOpToAtomic(innerOp);
1138+
if (!llvm::is_contained(innerOp.getOperands(),
1139+
opInst.getRegion().getArgument(0)))
1140+
continue;
1141+
isRegionArgUsed = true;
1142+
isXBinopExpr = innerOp.getNumOperands() > 0 &&
1143+
innerOp.getOperand(0) == opInst.getRegion().getArgument(0);
1144+
mlirExpr = (isXBinopExpr ? innerOp.getOperand(1) : innerOp.getOperand(0));
1145+
break;
1146+
}
1147+
}
1148+
if (!isRegionArgUsed)
1149+
return opInst.emitError("no atomic update operation with region argument"
1150+
" as operand found inside atomic.update region");
11481151

1149-
mlir::Value mlirExpr = (isXBinopExpr ? innerUpdateOp.getOperand(1)
1150-
: innerUpdateOp.getOperand(0));
11511152
llvm::Value *llvmExpr = moduleTranslation.lookupValue(mlirExpr);
11521153
llvm::Value *llvmX = moduleTranslation.lookupValue(opInst.getX());
11531154
llvm::Type *llvmXElementType = moduleTranslation.convertType(
@@ -1210,25 +1211,28 @@ convertOmpAtomicCapture(omp::AtomicCaptureOp atomicCaptureOp,
12101211
isPostfixUpdate = atomicCaptureOp.getSecondOp() ==
12111212
atomicCaptureOp.getAtomicUpdateOp().getOperation();
12121213
auto &innerOpList = atomicUpdateOp.getRegion().front().getOperations();
1213-
if (innerOpList.size() != 2)
1214-
return atomicUpdateOp.emitError(
1215-
"exactly two operations are allowed inside an "
1216-
"atomic update region while lowering to LLVM IR");
1217-
Operation *innerUpdateOp = atomicUpdateOp.getFirstOp();
1218-
if (innerUpdateOp->getNumOperands() != 2 ||
1219-
!llvm::is_contained(innerUpdateOp->getOperands(),
1220-
atomicUpdateOp.getRegion().getArgument(0)))
1214+
bool isRegionArgUsed{false};
1215+
// Find the binary update operation that uses the region argument
1216+
// and get the expression to update
1217+
for (Operation &innerOp : innerOpList) {
1218+
if (innerOp.getNumOperands() == 2) {
1219+
binop = convertBinOpToAtomic(innerOp);
1220+
if (!llvm::is_contained(innerOp.getOperands(),
1221+
atomicUpdateOp.getRegion().getArgument(0)))
1222+
continue;
1223+
isRegionArgUsed = true;
1224+
isXBinopExpr =
1225+
innerOp.getNumOperands() > 0 &&
1226+
innerOp.getOperand(0) == atomicUpdateOp.getRegion().getArgument(0);
1227+
mlirExpr =
1228+
(isXBinopExpr ? innerOp.getOperand(1) : innerOp.getOperand(0));
1229+
break;
1230+
}
1231+
}
1232+
if (!isRegionArgUsed)
12211233
return atomicUpdateOp.emitError(
1222-
"the update operation inside the region must be a binary operation "
1223-
"and that update operation must have the region argument as an "
1224-
"operand");
1225-
binop = convertBinOpToAtomic(*innerUpdateOp);
1226-
1227-
isXBinopExpr = innerUpdateOp->getOperand(0) ==
1228-
atomicUpdateOp.getRegion().getArgument(0);
1229-
1230-
mlirExpr = (isXBinopExpr ? innerUpdateOp->getOperand(1)
1231-
: innerUpdateOp->getOperand(0));
1234+
"no atomic update operation with region argument"
1235+
" as operand found inside atomic.update region");
12321236
}
12331237

12341238
llvm::Value *llvmExpr = moduleTranslation.lookupValue(mlirExpr);

‎mlir/test/Target/LLVMIR/openmp-llvm-invalid.mlir‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,6 @@
33
// Checking translation when the update is carried out by using more than one op
44
// in the region.
55
llvm.func @omp_atomic_update_multiple_step_update(%x: !llvm.ptr<i32>, %expr: i32) {
6-
// expected-error @+2 {{exactly two operations are allowed inside an atomic update region while lowering to LLVM IR}}
7-
// expected-error @+1 {{LLVM Translation failed for operation: omp.atomic.update}}
86
omp.atomic.update %x : !llvm.ptr<i32> {
97
^bb0(%xval: i32):
108
%t1 = llvm.mul %xval, %expr : i32
@@ -20,7 +18,7 @@ llvm.func @omp_atomic_update_multiple_step_update(%x: !llvm.ptr<i32>, %expr: i32
2018
// Checking translation when the captured variable is not used in the inner
2119
// update operation
2220
llvm.func @omp_atomic_update_multiple_step_update(%x: !llvm.ptr<i32>, %expr: i32) {
23-
// expected-error @+2 {{the update operation inside the region must be a binary operation and that update operation must have the region argument as an operand}}
21+
// expected-error @+2 {{no atomic update operation with region argument as operand found inside atomic.update region}}
2422
// expected-error @+1 {{LLVM Translation failed for operation: omp.atomic.update}}
2523
omp.atomic.update %x : !llvm.ptr<i32> {
2624
^bb0(%xval: i32):
@@ -38,7 +36,7 @@ llvm.func @omp_atomic_update_multiple_step_update(%x: !llvm.ptr<i32>, %v: !llvm.
3836
// expected-error @+1 {{LLVM Translation failed for operation: omp.atomic.capture}}
3937
omp.atomic.capture memory_order(seq_cst) {
4038
omp.atomic.read %v = %x : !llvm.ptr<i32>, i32
41-
// expected-error @+1 {{the update operation inside the region must be a binary operation and that update operation must have the region argument as an operand}}
39+
// expected-error @+1 {{no atomic update operation with region argument as operand found inside atomic.update region}}
4240
omp.atomic.update %x : !llvm.ptr<i32> {
4341
^bb0(%xval: i32):
4442
%newval = llvm.mul %expr, %expr : i32
@@ -53,10 +51,8 @@ llvm.func @omp_atomic_update_multiple_step_update(%x: !llvm.ptr<i32>, %v: !llvm.
5351
// Checking translation when the captured variable is not used in the inner
5452
// update operation
5553
llvm.func @omp_atomic_update_multiple_step_update(%x: !llvm.ptr<i32>, %v: !llvm.ptr<i32>, %expr: i32) {
56-
// expected-error @+1 {{LLVM Translation failed for operation: omp.atomic.capture}}
5754
omp.atomic.capture memory_order(seq_cst) {
58-
omp.atomic.read %v = %x : !llvm.ptr<i32>, i32
59-
// expected-error @+1 {{exactly two operations are allowed inside an atomic update region while lowering to LLVM IR}}
55+
omp.atomic.read %v = %x : !llvm.ptr<i32>, i32
6056
omp.atomic.update %x : !llvm.ptr<i32> {
6157
^bb0(%xval: i32):
6258
%t1 = llvm.mul %xval, %expr : i32

0 commit comments

Comments
 (0)