[FIRRTL][InferWidths] Fix back-prop, fix ref equality, fix upper bound. #5403
Conversation
| // This should only happen if somehow the constraint is | ||
| // solved before visiting this expression, so that our upperBound | ||
| // was not added to the worklist such that it was handled first. | ||
| if (expr->upperBound && solvedExprs.contains(expr->upperBound)) |
There was a problem hiding this comment.
I'm not sure we don't need to handle this re:seenVars and checkCycles/etc.?
Or if constraint expressions can be re-used such that we could have solution for the constraint w/o having put the upper bound on the worklist first?
| @@ -1819,7 +1821,7 @@ void InferenceMapping::constrainTypes(Value larger, Value smaller) { | |||
| } else if (type.isGround()) { | |||
| // Leaf element, look up their expressions, and create the constraint. | |||
| constrainTypes(getExpr(FieldRef(larger, fieldID)), | |||
| getExpr(FieldRef(smaller, fieldID))); | |||
| getExpr(FieldRef(smaller, fieldID)), false, equal); | |||
There was a problem hiding this comment.
Don't love this re:bools and ergonomics but wanted to get this reviewed for functionality first, but once that seems good suggestions on improvements welcome (please!). FWIW-- imposeUpperBounds adds LEQ in compatible way to GEQ (if x >= y is requested, it'll add y <= x if x isn't free variable), where-as "equal" causes LEQ (x >= y && x <= y) to be added. Anyway.
|
Note that the discussion on whether |
There was a problem hiding this comment.
This looks good to me! Since this is mostly touching the upperBound, which isn't used for most of the widths, this shouldn't have any impact on existing width inference.
I wish there was a saner way to do width inference overall than building this shadow expression graph and then running this custom solver. Problem for another day 😉
FWIW this explicitly fixes a bug where we inferred wrong widths, not involving upper bound, mostly because the operation-visiting code put constraints in both directions so we propagated the wrong way (perhaps because didn't have a way to express what it wanted re:new |
dtzSiFive
deleted the
fix/infer-widths/backprop-upperbound-and-refs-oh-my
branch
|
tyvm for review and discussion/explanation! |
Fix upperBoundSolution being set from bool indicating if cycle, instead of the value solved for (.first instead of .second).
Tweak how/when upper bound constraint is solved.
Fixes #5002.
Fixes #5391.