Skip to content

Commit

Permalink
[FIRRTL] LowerToHW: guard against folded operations
Browse files Browse the repository at this point in the history
In LowerToHW we copy the name from FIRRTL ops to the lowered HW ops.  We
often call `createOrFold` when creating the HW ops, which can mean that
there is no operation to copy the name to.  We were not properly
detecting this scenario, which could cause us to try to copy the name to
a null operation.  Most ops were already covered under a guard in a
generic code path, but div-like and subaccess-like operations needed to
be guarded.  Some of these problem are impossible to test right now as
we require folders to be implemented for some operations.
  • Loading branch information
youngar committed Jul 19, 2024
1 parent 846139b commit ee34bc1
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 5 deletions.
12 changes: 8 additions & 4 deletions lib/Conversion/FIRRTLToHW/LowerToHW.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2729,7 +2729,8 @@ FailureOr<Value> FIRRTLLowering::lowerSubindex(SubindexOp op, Value input) {
result = builder.createOrFold<sv::ArrayIndexInOutOp>(input, iIdx);
else
result = builder.createOrFold<hw::ArrayGetOp>(input, iIdx);
tryCopyName(result.getDefiningOp(), op);
if (auto *definingOp = result.getDefiningOp())
tryCopyName(definingOp, op);
return result;
}

Expand All @@ -2752,7 +2753,8 @@ FailureOr<Value> FIRRTLLowering::lowerSubaccess(SubaccessOp op, Value input) {
result = builder.createOrFold<sv::ArrayIndexInOutOp>(input, valueIdx);
else
result = createArrayIndexing(input, valueIdx);
tryCopyName(result.getDefiningOp(), op);
if (auto definingOp = result.getDefiningOp())
tryCopyName(definingOp, op);
return result;
}

Expand All @@ -2772,7 +2774,8 @@ FailureOr<Value> FIRRTLLowering::lowerSubfield(SubfieldOp op, Value input) {
result = builder.createOrFold<sv::StructFieldInOutOp>(input, field);
else
result = builder.createOrFold<hw::StructExtractOp>(input, field);
tryCopyName(result.getDefiningOp(), op);
if (auto *definingOp = result.getDefiningOp())
tryCopyName(definingOp, op);
return result;
}

Expand Down Expand Up @@ -3635,7 +3638,8 @@ LogicalResult FIRRTLLowering::lowerDivLikeOp(Operation *op) {
else
result = builder.createOrFold<UnsignedOp>(lhs, rhs, true);

tryCopyName(result.getDefiningOp(), op);
if (auto *definingOp = result.getDefiningOp())
tryCopyName(definingOp, op);

if (resultType == opType)
return setLowering(op->getResult(0), result);
Expand Down
6 changes: 5 additions & 1 deletion test/Conversion/FIRRTLToHW/lower-to-hw.mlir
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,11 @@ firrtl.circuit "Simple" attributes {annotations = [{class =
// CHECK-NEXT: = comb.extract [[SHIFT]] from 0 : (i14) -> i4
%31 = firrtl.dshr %25, %18 : (!firrtl.sint<4>, !firrtl.uint<14>) -> !firrtl.sint<4>

// CHECK-NEXT: comb.icmp bin ule {{.*}}, {{.*}} : i4
// Noop.
%c0_ui1 = firrtl.constant 0 : !firrtl.const.uint<1>
%32 = firrtl.dshr %in1, %c0_ui1 { name = "test" } : (!firrtl.uint<4>, !firrtl.const.uint<1>) -> !firrtl.uint<4>

// CHECK: comb.icmp bin ule {{.*}}, {{.*}} : i4
%41 = firrtl.leq %in1, %4 : (!firrtl.uint<4>, !firrtl.uint<4>) -> !firrtl.uint<1>
// CHECK-NEXT: comb.icmp bin ult {{.*}}, {{.*}} : i4
%42 = firrtl.lt %in1, %4 : (!firrtl.uint<4>, !firrtl.uint<4>) -> !firrtl.uint<1>
Expand Down

0 comments on commit ee34bc1

Please sign in to comment.