Conversation
Eliminate implicit truncations from FIRRTL. This changes connects, InferWidths, and parsing.
|
This version is more aggressive with simplifying more locations in the code. Especially watching emitConnect changes in large design testing. |
There was a problem hiding this comment.
Parser rejections are great as the errors are better. However, this should also have verifiers do the rejection, too, right?
There was a problem hiding this comment.
This NFC change that was really a test mistake (a FIRRTL or MLIR file relying on implicit truncation that didn't need to and wasn't testing implicit truncation) could be broken out and landed directly. I'm thinking about this as I'd like to keep this change out of the 1.145.0 release as I am pretty sure this is going to need Chisel and internal codebase coordination to not break a ton of stuff.
However, it would be great to start prepping CIRCT for this change.
| FIRRTL version 4.0.0 | ||
| FIRRTL version 4.0.0 |
| input inp_1: UInt<5> | ||
| input inp_2: UInt<5> | ||
| input inp_3: UInt<5> | ||
| output out1: UInt<10> | ||
| output out2: UInt<10> | ||
| input inp_1: UInt<6> | ||
| input inp_2: UInt<6> | ||
| input inp_3: UInt<6> | ||
| output out1: UInt<12> | ||
| output out2: UInt<12> |
| // Issue #1088 - Implicit truncation is not allowed. | ||
| firrtl.circuit "Issue1088" { | ||
| firrtl.module @Issue1088(out %y: !firrtl.sint<4>) { | ||
| %x = firrtl.wire : !firrtl.sint | ||
| %c200_si = firrtl.constant 200 : !firrtl.sint | ||
| // expected-error @+1 {{destination '!firrtl.sint<4>' is not as wide as the source '!firrtl.sint<9>'}} | ||
| firrtl.connect %y, %x : !firrtl.sint<4>, !firrtl.sint | ||
| firrtl.connect %x, %c200_si : !firrtl.sint, !firrtl.sint | ||
| } | ||
| } |
| // The source can be extended if needed, but truncation is not allowed. | ||
| assert(dstWidth >= srcWidth && | ||
| "implicit truncation is not allowed in connect operations"); |
There was a problem hiding this comment.
The addition of these asserts likely means that emitConnect needs to return a LogicalResult for better handling of this. I'm worried that this will be brittle in release builds unless we can guarantee that this is never going to ever occur.
Alternatively, if there is a ConnectOp verifier, that would be entirely fine. (That is probably the right thing to do here and then maybe leave the asserts so that this errors faster in debug/relwithdebinfo builds.)
There was a problem hiding this comment.
Was looking at that. This is more a thing to see how often this triggers and under what conditions.
| // If there's no access operation, skip this port | ||
| if (!cmemoryPortAccess) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
This looks unrelated.
Also, nit:
| // If there's no access operation, skip this port | |
| if (!cmemoryPortAccess) { | |
| continue; | |
| } | |
| // If there's no access operation, skip this port | |
| if (!cmemoryPortAccess) | |
| continue; |
| if (addrWidth >= 0 && indexWidth >= 0 && indexWidth > addrWidth) { | ||
| indexValue = | ||
| TailPrimOp::create(portBuilder, indexValue, indexWidth - addrWidth) | ||
| .getResult(); | ||
| } |
There was a problem hiding this comment.
| if (addrWidth >= 0 && indexWidth >= 0 && indexWidth > addrWidth) { | |
| indexValue = | |
| TailPrimOp::create(portBuilder, indexValue, indexWidth - addrWidth) | |
| .getResult(); | |
| } | |
| if (addrWidth >= 0 && indexWidth >= 0 && indexWidth > addrWidth) | |
| indexValue = | |
| TailPrimOp::create(portBuilder, indexValue, indexWidth - addrWidth) | |
| .getResult(); |
uenoku
changed the title
3 participants
Eliminate implicit truncations from FIRRTL. This changes connects, InferWidths, and parsing.
Assisted-by: Augment:Sonet-4.5