[FIRRTLUtils] Fix walkDrivers subfield id calculation #7536
Conversation
test/Dialect/FIRRTL/sfc-compat.mlir
Outdated
| %foo = firrtl.wire : !firrtl.vector<uint<1>, 1> | ||
| %0 = firrtl.subindex %foo[0] : !firrtl.vector<uint<1>, 1> | ||
| %c1_ui1 = firrtl.constant 1 : !firrtl.uint<1> | ||
| firrtl.matchingconnect %0, %c1_ui1 : !firrtl.uint<1> | ||
| %bar = firrtl.wire : !firrtl.vector<vector<uint<1>, 1>, 1> | ||
| %1 = firrtl.subindex %bar[0] : !firrtl.vector<vector<uint<1>, 1>, 1> | ||
| %2 = firrtl.subindex %foo[0] : !firrtl.vector<uint<1>, 1> | ||
| %3 = firrtl.subindex %1[0] : !firrtl.vector<uint<1>, 1> | ||
| firrtl.matchingconnect %3, %2 : !firrtl.uint<1> | ||
| // Check that firrtl.regreset is not transformed into reg op if wire is not invalid | ||
| // CHECK: firrtl.regreset | ||
| %x = firrtl.regreset %clock, %reset, %bar : !firrtl.clock, !firrtl.uint<1>, !firrtl.vector<vector<uint<1>, 1>, 1>, !firrtl.vector<vector<uint<1>, 1>, 1> | ||
| %4 = firrtl.subindex %x[0] : !firrtl.vector<vector<uint<1>, 1>, 1> | ||
| %5 = firrtl.subindex %4[0] : !firrtl.vector<uint<1>, 1> | ||
| firrtl.matchingconnect %5, %5 : !firrtl.uint<1> | ||
| } |
There was a problem hiding this comment.
Mega-nit: this test is a bit verbose and could be worth golfing down for maintainability.
| // If fieldID is zero, this points to entire subfields. | ||
| if (fieldID == 0) | ||
| workStack.emplace_back(subOriginal, subRef, value, 0); | ||
| else { | ||
| assert(fieldID >= subID); | ||
| workStack.emplace_back(subOriginal, subRef, value, fieldID - subID); | ||
| } |
There was a problem hiding this comment.
There might be the case that fieldID < subID so this fix may not work too.. :) Thank you @dtzWill for pointing it out!
|
Trying to refresh my memory on how this code works - still pretty hazy but I think your fix makes sense. Once the |
|
I was not sure and the test case from the issue IIRC didn't run into a case where either was true (the Thanks! |
|
Thank you for review! |
This fixes an overflow in walkDrivers when field ID is zero.
Fix chipsalliance/chisel#4354 and #7423.