Skip to content

Commit

Permalink
[Seq] Remove incorrect canonicalization and reject registers with pre…
Browse files Browse the repository at this point in the history
…sets(#7289)

This PR removes a canonicalization that incorrectly replaces self-connect registers with their non-constant reset values.
This PR also adds additional conditions for presets. We have to revisit to handle preset values. 

Fix #7266.
  • Loading branch information
uenoku authored Jul 9, 2024
1 parent 2e17355 commit 3f09e3d
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 19 deletions.
30 changes: 15 additions & 15 deletions lib/Dialect/Seq/SeqOps.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -547,6 +547,11 @@ void FirRegOp::getAsmResultNames(OpAsmSetValueNameFn setNameFn) {
std::optional<size_t> FirRegOp::getTargetResultIndex() { return 0; }

LogicalResult FirRegOp::canonicalize(FirRegOp op, PatternRewriter &rewriter) {
// Don't canonicalize if there is a preset value for now.
// TODO: Handle a preset value.
if (op.getPresetAttr())
return failure();

// If the register has a constant zero reset, drop the reset and reset value
// altogether.
if (auto reset = op.getReset()) {
Expand Down Expand Up @@ -576,20 +581,14 @@ LogicalResult FirRegOp::canonicalize(FirRegOp op, PatternRewriter &rewriter) {
return false;
};

if (isConstant()) {
if (auto resetValue = op.getResetValue()) {
// If the register has a reset value, we can replace it with that.
rewriter.replaceOp(op, resetValue);
if (isConstant() && !op.getResetValue()) {
if (isa<seq::ClockType>(op.getType())) {
rewriter.replaceOpWithNewOp<seq::ConstClockOp>(
op, seq::ClockConstAttr::get(rewriter.getContext(), ClockConst::Low));
} else {
if (isa<seq::ClockType>(op.getType())) {
rewriter.replaceOpWithNewOp<seq::ConstClockOp>(
op,
seq::ClockConstAttr::get(rewriter.getContext(), ClockConst::Low));
} else {
auto constant = rewriter.create<hw::ConstantOp>(
op.getLoc(), APInt::getZero(hw::getBitWidth(op.getType())));
rewriter.replaceOpWithNewOp<hw::BitcastOp>(op, op.getType(), constant);
}
auto constant = rewriter.create<hw::ConstantOp>(
op.getLoc(), APInt::getZero(hw::getBitWidth(op.getType())));
rewriter.replaceOpWithNewOp<hw::BitcastOp>(op, op.getType(), constant);
}
return success();
}
Expand Down Expand Up @@ -652,8 +651,9 @@ LogicalResult FirRegOp::canonicalize(FirRegOp op, PatternRewriter &rewriter) {
}

OpFoldResult FirRegOp::fold(FoldAdaptor adaptor) {
// If the register has a symbol, we can't optimize it away.
if (getInnerSymAttr())
// If the register has a symbol or preset value, we can't optimize it away.
// TODO: Handle a preset value.
if (getInnerSymAttr() || getPresetAttr())
return {};

// If the register is held in permanent reset, replace it with its reset
Expand Down
15 changes: 11 additions & 4 deletions test/Dialect/Seq/canonicalization.mlir
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,13 @@ hw.module @FirRegReset(in %clk : !seq.clock, in %in : i32, in %r : i1, in %v : i
%true = hw.constant true

// Registers that update to themselves should be replaced with their reset
// value.
%reg0 = seq.firreg %reg0 clock %clk reset sync %r, %v : i32
hw.instance "reg0" @Observe(x: %reg0: i32) -> ()
// CHECK: hw.instance "reg0" @Observe(x: %v: i32) -> ()
// value only when a reset value is constant.
%reg0a = seq.firreg %reg0a clock %clk reset sync %r, %v : i32
%reg0b = seq.firreg %reg0b clock %clk reset sync %r, %c0_i32 : i32
hw.instance "reg0a" @Observe(x: %reg0a: i32) -> ()
hw.instance "reg0b" @Observe(x: %reg0b: i32) -> ()
// CHECK: hw.instance "reg0a" @Observe(x: %reg0a: i32) -> ()
// CHECK-NEXT: hw.instance "reg0b" @Observe(x: %c0_i32: i32) -> ()

// Registers that never reset should drop their reset value.
%reg1 = seq.firreg %in clock %clk reset sync %false, %v : i32
Expand Down Expand Up @@ -72,6 +75,10 @@ hw.module @FirRegReset(in %clk : !seq.clock, in %in : i32, in %r : i1, in %v : i
// CHECK: hw.instance "reg3a" @Observe(x: %reg3a: i32) -> ()
// CHECK: hw.instance "reg3b" @Observe(x: %reg3b: i32) -> ()
// CHECK: hw.instance "reg3c" @Observe(x: %c0_i32: i32) -> ()

// A register with preset value is not folded right now
// CHECK: %reg_preset = seq.firreg
%reg_preset = seq.firreg %reg_preset clock %clk reset sync %r, %c0_i32 preset 3: i32
}

// CHECK-LABEL: @FirRegAggregate
Expand Down

0 comments on commit 3f09e3d

Please sign in to comment.