[FIRRTL] Handle flips in const connections #5210
Conversation
|
I think we still accept partial connects, its probably best to update |
|
|
||
| // ----- | ||
|
|
||
| /// Non-const doube flip types cannot be connected to const. |
lib/Dialect/FIRRTL/FIRRTLTypes.cpp
Outdated
| // If the types are trivially equal, return true. | ||
| if (destFType == srcFType) | ||
| return true; | ||
| // If the types are trivially equal, return true as long as outer constness is | ||
| // compatible. | ||
| if (destFType == srcFType) { | ||
| if (destOuterTypeIsConst == srcOuterTypeIsConst) | ||
| return true; | ||
| if (destFType.isGround()) | ||
| return !destOuterTypeIsConst || srcOuterTypeIsConst; | ||
| } |
There was a problem hiding this comment.
Hmm maybe we should just get rid of the early break. I think I added this recently, and introduced the bug of ignoring the outer const type. If we ever push const to the leaves or something like that, then maybe we can introduce the early break again, wdyt?
Also I suggest we add a test case for this because I keep thinking we can add this trivial early break. Its now covered by the flip test cases you added, but the most basic test might be:
/// Const types cannot be connected to non-const types.
firrtl.circuit "test" {
firrtl.module @test(in %in : !firrtl.const.bundle<a: uint<1>>,
out %out : !firrtl.bundle<a: uint<1>>) {
// expected-error @+1 {{type mismatch}}
firrtl.connect %out, %in : !firrtl.bundle<a: uint<1>>, !firrtl.const.bundle<a: uint<1>>
}
}There was a problem hiding this comment.
Yeah, the outer constness gets kind of unwieldy, I like your suggestion of keeping things in one place for ground types.
Removed, and added test case (I assume you accidentally wrote the valid version).
lib/Dialect/FIRRTL/FIRRTLTypes.cpp
Outdated
| // A 'const' ground destination requires a 'const' source. | ||
| if (destType.isGround() && destIsConst && !srcIsConst) | ||
| return false; |
There was a problem hiding this comment.
Maybe we should keep all the ground-related logic grouped at the end? The current logic down below sort of looks wrong to me if this were to fall through:
// Ground types can be connected if they are the same, or the source type is
// a const version of the destination type.
return destType == srcType || destType == srcType.getConstType(false);Should this be something like:
if (destIsConst && !srcIsConst)
return false;
return destType.getConst(false) == srcType.getConst(false);
// OR:
auto realDestType = destType.getConstType(destIsConst);
return realDestType == srcType.getConstType(true) || realDestType == srcType.getConstType(false);There was a problem hiding this comment.
I grouped the ground stuff at the end, and I also moved the reset-type tests below the aggregates as well.
Also fix a partial connect reset flip edge case
|
I updated |
Addresses #5209
I can also update
areTypesWeaklyEquivalent, althoughconsttypes were introduced into the firrtl spec in the same version that removed partial connection, so I suppose I can leave it. Or I can remove all of the const logic it has to clean things up.