terapines
/
circt
mirror of https://github.com/llvm/circt.git
2
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

[comb] Remove the comb.merge operation and supporting logic. 

Schuyler points out in Issue #1600 that it isn't correctly implemented,
and there is only one place in the compiler that forms it ... in a
theoretical case.  I added this a long time ago on a theoretical basis.
It is best to remove this until there is a real need for it.

This fixes Issue #1600.
This commit is contained in:
Chris Lattner 2021-08-17 22:31:51 -07:00
parent a22c21e19b
commit f0fa80b451
10 changed files with 29 additions and 150 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

15
include/circt/Dialect/Comb/Combinational.td
View File

@ -85,21 +85,6 @@ def XorOp : UTVariadicOp<"xor", [Commutative]> {
}];
}
def MergeOp : UTVariadicOp<"merge", [Commutative]> {
let summary ="Electrically merge signals together as if connected by a wire.";
let description = [{
This operation merges multiple signals together as if they were connected to
a single wire. Presumably at most one of them should be a Z state at any
given time otherwise you'd get a short circuit.
This operation allows modeling multiconnect semantics in SSA.
```
%result = hw.merge %a, %b, %c : t1
```
}];
let hasCanonicalizeMethod = false;
}
//===----------------------------------------------------------------------===//
// Comparisons
//===----------------------------------------------------------------------===//

3
integration_test/Bindings/Python/dialects/comb.py
View File

@ -172,9 +172,6 @@ with Context() as ctx, Location.unknown():
# CHECK: comb.xor %[[CONST]], %[[CONST]]
comb.XorOp.create(const.result, const.result)
# CHECK: comb.merge %[[CONST]], %[[CONST]]
comb.MergeOp.create(const.result, const.result)
# CHECK: comb.concat %[[CONST]], %[[CONST]]
comb.ConcatOp.create(i32, const.result, const.result)

5
lib/Bindings/Python/circt/dialects/_comb_ops_ext.py
View File

@ -185,11 +185,6 @@ class XorOp:
pass
@VariadicOp
class MergeOp:
pass
# Sugar classes for miscellaneous ops.
class ConcatOp:

55
lib/Conversion/FIRRTLToHW/LowerToHW.cpp
View File

@ -779,19 +779,21 @@ static Value tryEliminatingConnectsToValue(Value flipValue,
if (flipValue.getType().isa<AnalogType>())
return tryEliminatingAttachesToAnalogValue(flipValue, insertPoint);
SmallVector<ConnectOp, 2> connects;
ConnectOp theConnect;
for (auto *use : flipValue.getUsers()) {
// We only know about 'connect' uses, where this is the destination.
auto connect = dyn_cast<ConnectOp>(use);
if (!connect || connect.src() == flipValue)
if (!connect || connect.src() == flipValue ||
// We only support things with a single connect.
theConnect)
return {};
connects.push_back(connect);
theConnect = connect;
}
// We don't have an HW equivalent of "poison" so just don't special case
// the case where there are no connects other uses of an output.
if (connects.empty())
if (!theConnect)
return {}; // TODO: Emit an sv.constantz here since it is unconnected.
// Don't special case zero-bit results.
@ -801,40 +803,31 @@ static Value tryEliminatingConnectsToValue(Value flipValue,
// Convert each connect into an extended version of its operand being
// output.
SmallVector<Value, 2> results;
ImplicitLocOpBuilder builder(insertPoint->getLoc(), insertPoint);
for (auto connect : connects) {
auto connectSrc = connect.src();
auto connectSrc = theConnect.src();
// Convert fliped sources to passive sources.
if (!connectSrc.getType().cast<FIRRTLType>().isPassive())
connectSrc = builder.createOrFold<AsPassivePrimOp>(connectSrc);
// Convert fliped sources to passive sources.
if (!connectSrc.getType().cast<FIRRTLType>().isPassive())
connectSrc = builder.createOrFold<AsPassivePrimOp>(connectSrc);
// We know it must be the destination operand due to the types, but the
// source may not match the destination width.
auto destTy = flipValue.getType().cast<FIRRTLType>().getPassiveType();
if (destTy.getBitWidthOrSentinel() !=
connectSrc.getType().cast<FIRRTLType>().getBitWidthOrSentinel()) {
// The only type mismatchs we care about is due to integer width
// differences.
auto destWidth = destTy.getBitWidthOrSentinel();
assert(destWidth != -1 && "must know integer widths");
connectSrc =
builder.createOrFold<PadPrimOp>(destTy, connectSrc, destWidth);
}
// Remove the connect and use its source as the value for the output.
connect.erase();
results.push_back(connectSrc);
// We know it must be the destination operand due to the types, but the
// source may not match the destination width.
auto destTy = flipValue.getType().cast<FIRRTLType>().getPassiveType();
if (destTy.getBitWidthOrSentinel() !=
connectSrc.getType().cast<FIRRTLType>().getBitWidthOrSentinel()) {
// The only type mismatchs we care about is due to integer width
// differences.
auto destWidth = destTy.getBitWidthOrSentinel();
assert(destWidth != -1 && "must know integer widths");
connectSrc = builder.createOrFold<PadPrimOp>(destTy, connectSrc, destWidth);
}
// Convert from FIRRTL type to builtin type to do the merge.
for (auto &result : results)
result = castFromFIRRTLType(result, loweredType, builder);
// Remove the connect and use its source as the value for the output.
theConnect.erase();
// Folding merge of one value just returns the value.
return builder.createOrFold<comb::MergeOp>(results);
// Convert from FIRRTL type to builtin type.
return castFromFIRRTLType(connectSrc, loweredType, builder);
}
static SmallVector<SubfieldOp> getAllFieldAccesses(Value structValue,

8
lib/Dialect/Comb/CombFolds.cpp
View File

@ -962,14 +962,6 @@ LogicalResult XorOp::canonicalize(XorOp op, PatternRewriter &rewriter) {
return failure();
}
OpFoldResult MergeOp::fold(ArrayRef<Attribute> constants) {
// hw.merge(x, x, x) -> x.
if (llvm::all_of(inputs(), [&](auto in) { return in == this->inputs()[0]; }))
return inputs()[0];
return {};
}
OpFoldResult SubOp::fold(ArrayRef<Attribute> constants) {
APInt value;
// sub(x - 0) -> x

31
lib/Translation/ExportVerilog/ExportVerilog.cpp
View File

@ -3063,29 +3063,6 @@ static void lowerAlwaysInlineOperation(Operation *op) {
return;
}
/// We lower the Merge operation to a wire at the top level along with
/// assigns to it and a ReadInOut.
static Value lowerMergeOp(MergeOp merge) {
auto module = merge->getParentOfType<HWModuleOp>();
assert(module && "merges should only be in a module");
// Start with the wire at the top level.
ImplicitLocOpBuilder b(merge.getLoc(), &module.getBodyBlock()->front());
auto wire = b.create<WireOp>(merge.getType());
// Each of the operands is an assign or passign into the wire.
b.setInsertionPoint(merge);
if (merge->getParentOp()->hasTrait<sv::ProceduralRegion>()) {
for (auto op : merge.getOperands())
b.create<PAssignOp>(wire, op);
} else {
for (auto op : merge.getOperands())
b.create<AssignOp>(wire, op);
}
return b.create<ReadInOutOp>(wire);
}
/// Lower a commutative operation into an expression tree. This enables
/// long-line splitting to work with them.
static Value lowerVariadicCommutativeOp(Operation &op, OperandRange operands) {
@ -3353,14 +3330,6 @@ static void prepareHWModule(Block &block, ModuleNameManager &names,
continue;
}
// Lower 'merge' operations to wires and connects.
if (auto merge = dyn_cast<MergeOp>(op)) {
auto result = lowerMergeOp(merge);
op.getResult(0).replaceAllUsesWith(result);
op.erase();
continue;
}
// Lower commutative variadic operations with more than two operands into
// balanced operand trees so we can split long lines across multiple
// statements.

8
test/Conversion/FIRRTLToHW/lower-to-hw-module.mlir
View File

@ -100,6 +100,8 @@ firrtl.circuit "Simple" {
out %outD: !firrtl.uint<4>,
in %inE: !firrtl.uint<3>,
out %outE: !firrtl.uint<4>) {
// CHECK: %.outB.output = sv.wire : !hw.inout<i4>
// CHECK: [[OUTBR:%.+]] = sv.read_inout %.outB.output
// CHECK: [[OUTC:%.+]] = sv.wire : !hw.inout<i4>
// CHECK: [[OUTCR:%.+]] = sv.read_inout %.outC.output
// CHECK: [[OUTD:%.+]] = sv.wire : !hw.inout<i4>
@ -110,7 +112,9 @@ firrtl.circuit "Simple" {
// Multi connect
firrtl.connect %outB, %inA : !firrtl.uint<4>, !firrtl.uint<4>
// CHECK: sv.assign %.outB.output, %inA : i4
firrtl.connect %outB, %inB : !firrtl.uint<4>, !firrtl.uint<4>
// CHECK: sv.assign %.outB.output, %inB : i4
%0 = firrtl.sub %inA, %outC : (!firrtl.uint<4>, !firrtl.uint<4>) -> !firrtl.uint<5>
@ -118,11 +122,9 @@ firrtl.circuit "Simple" {
firrtl.connect %outE, %inE : !firrtl.uint<4>, !firrtl.uint<3>
// CHECK: [[OUTBY:%.+]] = comb.merge %inB, %inA : i4
// Extension for outE
// CHECK: [[OUTE:%.+]] = comb.concat %false, %inE : (i1, i3) -> i4
// CHECK: hw.output %inA, [[OUTBY]], [[OUTCR]], [[OUTDR]], [[OUTE]]
// CHECK: hw.output %inA, [[OUTBR]], [[OUTCR]], [[OUTDR]], [[OUTE]]
}
// CHECK-LABEL: hw.module @Analog(%a1: !hw.inout<i1>) -> (%outClock: i1) {

10
test/Dialect/HW/canonicalization.mlir
View File

@ -56,16 +56,6 @@ hw.module @xor_cstfold(%arg0: i7) -> (i7) {
hw.output %0 : i7
}
// CHECK-LABEL: hw.module @merge_fold
// CHECK-NEXT: %0 = comb.merge %arg0, %arg0, %arg1 : i7
// CHECK-NEXT: hw.output %arg0, %arg0, %0 : i7, i7, i7
hw.module @merge_fold(%arg0: i7, %arg1: i7) -> (i7, i7, i7) {
%a = comb.merge %arg0 : i7
%b = comb.merge %arg0, %arg0, %arg0 : i7
%c = comb.merge %arg0, %arg0, %arg1 : i7
hw.output %a, %b, %c: i7, i7, i7
}
// CHECK-LABEL: hw.module @add_cstfold(%arg0: i7) -> (i7) {
// CHECK-NEXT: %c15_i7 = hw.constant 15 : i7
// CHECK-NEXT: %0 = comb.add %arg0, %c15_i7 : i7

15
test/ExportVerilog/hw-dialect.mlir
View File

@ -351,21 +351,6 @@ hw.module @wires(%in4: i4, %in8: i8) -> (%a: i4, %b: i8, %c: i8) {
hw.output %wireout, %memout1, %memout2 : i4, i8, i8
}
// CHECK-LABEL: module merge
hw.module @merge(%in1: i4, %in2: i4, %in3: i4, %in4: i4) -> (%x: i4) {
// CHECK: wire [3:0] _T;
// CHECK: assign _T = in1 + in2;
%a = comb.add %in1, %in2 : i4
// CHECK-NEXT: assign _T = in2;
// CHECK-NEXT: assign _T = in3;
%b = comb.merge %a, %in2, %in3 : i4
// CHECK: assign x = _T + in4 + in4;
%c = comb.add %b, %in4, %in4 : i4
hw.output %c : i4
}
// CHECK-LABEL: module signs
hw.module @signs(%in1: i4, %in2: i4, %in3: i4, %in4: i4) {
%awire = sv.wire : !hw.inout<i4>

29
test/ExportVerilog/sv-dialect.mlir
View File

@ -361,35 +361,6 @@ hw.module @exprInlineTestIssue439(%clk: i1) {
}
}
// CHECK-LABEL: module issue439(
// https://github.com/llvm/circt/issues/439
hw.module @issue439(%in1: i1, %in2: i1) {
// CHECK: wire _T;
// CHECK: wire _T_0 = in1 | in2;
%clock = comb.or %in1, %in2 : i1
// CHECK-NEXT: always @(posedge _T_0)
sv.always posedge %clock {
// CHECK-NEXT: _T <= in1;
// CHECK-NEXT: _T <= in2;
%merged = comb.merge %in1, %in2 : i1
// CHECK-NEXT: $fwrite(32'h80000002, "Bye %x\n", _T);
sv.fwrite "Bye %x\n"(%merged) : i1
}
}
// CHECK-LABEL: module issue726(
// https://github.com/llvm/circt/issues/726
hw.module @issue726(%in1: i1, %in2: i1) -> (%out: i1) {
// CHECK: wire _T;
// CHECK: assign _T = in1;
// CHECK: assign _T = in2;
%merged = comb.merge %in1, %in2 : i1
// CHECK: assign out = _T;
hw.output %merged : i1
}
// https://github.com/llvm/circt/issues/595
// CHECK-LABEL: module issue595
hw.module @issue595(%arr: !hw.array<128xi1>) {