Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MooreToCore] Properly deal with OOB access in dyn_extract #8201

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[MooreToCore] Properly deal with OOB access in dyn_extract #8201

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion include/circt/Dialect/Moore/MooreOps.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1123,7 +1123,7 @@ def ExtractOp : MooreOp<"extract", [Pure]> {
def DynExtractOp : MooreOp<"dyn_extract", [Pure]> {
let description = [{
It's similar with extract, but it's used to select from a value
with a dynamic low bit.
with a dynamic low bit. The `lowBit` operand is always interpreted as unsigned.
}];
let arguments = (ins UnpackedType:$input, UnpackedType:$lowBit);
let results = (outs UnpackedType:$result);
Expand Down
20 changes: 19 additions & 1 deletion lib/Conversion/MooreToCore/MooreToCore.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -825,12 +825,30 @@ struct DynExtractOpConversion : public OpConversionPattern<DynExtractOp> {
op->getLoc());

if (isa<hw::ArrayType>(resultType)) {
// FIXME: for packed arrays, the part that is OOB must be 0 or X and the
// part overlapping with the range must match the values of the range;
// for unpacked arrays, if part of the accessed slice is OOB the entire
// result is 0 or X
rewriter.replaceOpWithNewOp<hw::ArraySliceOp>(op, resultType,
adaptor.getInput(), idx);
return success();
}

rewriter.replaceOpWithNewOp<hw::ArrayGetOp>(op, adaptor.getInput(), idx);
Value size = rewriter.create<hw::ConstantOp>(
op.getLoc(), adaptor.getLowBit().getType(), arrType.getNumElements());
Copy link
Member Author

Choose a reason for hiding this comment

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

That can cause problems if the lowBit bitwidth is too small to hold the array size.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch. In that case we might be guaranteed to always access in bounds?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes 👍
But I need to make sure this IR is not built in that case because the pass would crash.

Value isOOB = rewriter.create<comb::ICmpOp>(
op.getLoc(), ICmpPredicate::uge, adaptor.getLowBit(), size);
Value access =
rewriter.create<hw::ArrayGetOp>(op.getLoc(), adaptor.getInput(), idx);
int64_t bw = hw::getBitWidth(arrType.getElementType());
if (bw < 0)
return failure();

Value zeroValueRaw =
rewriter.create<hw::ConstantOp>(op.getLoc(), APInt(bw, 0));
Value zeroValue = rewriter.createOrFold<hw::BitcastOp>(
op.getLoc(), arrType.getElementType(), zeroValueRaw);
rewriter.replaceOpWithNewOp<comb::MuxOp>(op, isOOB, zeroValue, access);
Comment on lines +841 to +851
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks reasonable assuming that hw::ArrayGetOp has undefined behaviour (or undefined result) for out-of-bounds accesses. I don't remember off the top of my head whether out-of-bounds index in ArrayGetOp causes a potential segfault in the lowered LLVM IR… But it might be nice if hw::ArrayGetOp just produce an undefined result, but don't cause arbitrary undefined behaviour such as crashing the simulator. Then we can just do exactly what you did here: read the value and then discard it with a mux if it's out of bounds.

Copy link
Member Author

Choose a reason for hiding this comment

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

The documentation of the HW aggregate ops is generally a bit sparse, but I'd say a OOB hw.array_get produces an undefined result, but no immediate UB. The HWToLLVM lowering currently does not take this into account. I'll create an issue for that.

return success();
}

Expand Down
6 changes: 5 additions & 1 deletion test/Conversion/MooreToCore/basic.mlir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,11 @@ func.func @Expressions(%arg0: !moore.i1, %arg1: !moore.l1, %arg2: !moore.i6, %ar
// CHECK-NEXT: [[V23:%.+]] = comb.extract %c2_i32 from 0 : (i32) -> i3
// CHECK-NEXT: [[MAX:%.+]] = hw.constant -1 : i3
// CHECK-NEXT: [[V24:%.+]] = comb.mux [[V22]], [[V23]], [[MAX]] : i3
// CHECK-NEXT: hw.array_get %arg5[[[V24]]] : !hw.array<5xi32>
// CHECK-NEXT: [[C5:%.+]] = hw.constant 5 : i32
// CHECK-NEXT: [[V25:%.+]] = comb.icmp uge %c2_i32, [[C5]] : i32
// CHECK-NEXT: [[V26:%.+]] = hw.array_get %arg5[[[V24]]] : !hw.array<5xi32>
// CHECK-NEXT: [[C0:%.+]] = hw.constant 0 : i32
// CHECK-NEXT: [[V27:%.+]] = comb.mux [[V25]], [[C0]], [[V26]] : i32
moore.dyn_extract %arg5 from %2 : !moore.array<5 x i32>, !moore.i32 -> !moore.i32

// CHECK-NEXT: [[V21:%.+]] = comb.extract %c0_i32 from 0 : (i32) -> i32
Expand Down