[FIRRTL] Improve canonicalization involving `invalidvalue` #2251

fabianschuiki · 2021-11-29T11:50:13Z

This extends the FIRRTL fold and canonicalization patterns to treat invalidvalue as a zero if it is an input to a primary operation. This is inline with what the Scala implementation of the FIRRTL compiler does.

Doing this removes quite a few of the special cases where certain ops are aware of invalidvalues and will fold as if it was a zero. The addition of the getConstant, isConstantZero, and canonicalizePrimOp helpers allow for most folders and canonicalizers to not having to bother with invalidvalues, as these helpers implicitly map invalidvalues to zero constants.

This also subsumes the work done in #2278 and #2198, which selectively added handling for invalidvalues to the PadPrimOp.

The extension of the folders and canonicalizers now makes all the cases in invalid-reg-fail.fir pass, albeit some only after accepting that the MLIR implementation folds additional cases where the Scala one might just leave an operation untouched. These are marked with a <-- fixed; upstream to Scala FIRRTL impl? and should probably be upstreamed as additional canonicalizations on the Scala side as well.

Original PR:

This draft PR is an effort to make invalidvalue behave as zero during canonicalization, while still maintaining the firrtl.invalidvalue operations in the IR, which we use in a canonicalization of MuxPrimOp to properly do WhenOp expansion.

With a few additional helpers, we can make all the folds and canonicalizations work with invalidvalue as zero. However as a result some of the integration tests in invalid-reg-pass.fir start to fail, which I think indicates that firtool now folds things more aggressively than the Scala implementation.

I don't know if it's worthwhile pursuing this effort, or whether we should rather try to match the Scala impl exactly (which would make logical equivalence pass, but is pretty tricky because we have to be bug-for-bug compatible).

seldridge

My gut instinct is that folding more is fine, so long as the fold involves an invalid interpreted as zero. Any error of this form is likely a missing fold in the SFC. (The first failure in invalid-reg-pass.fir is related to "convert to signed/cvt" not folding away. This just seems to be missing as SFC doesn't touch cvt(UInt<1>(0)).) This type of deviation shouldn't change functional correctness except for pre-reset/cycle zero behavior.

Any fold involving invalid propagation (foo(_, invalid) -> invalid) needs to be handled extremely carefully as the SFC never does this. This type of interpretation can introduce functional differences in the output.

fabianschuiki · 2021-11-29T15:42:30Z

That makes a lot of sense @seldridge. That would mean some of the test cases in invalid-reg-pass.fir would have to be adapted for MFC being able to fold more stuff. We could translate these into issues on the FIRRTL repo as we adjust each case, to ensure we match SFC and MFC without having to pass up on optimization opportunities in MFC.

lattner

This approach looks good to me!

lib/Dialect/FIRRTL/FIRRTLFolds.cpp

lattner · 2021-11-29T19:20:24Z

lib/Dialect/FIRRTL/FIRRTLFolds.cpp

+  // InvalidValue inputs simply read as zero.
+  if (auto result = constant.dyn_cast_or_null<InvalidValueAttr>()) {
+    llvm::errs() << "Treating " << operand << " as zero value\n";
+    return APSInt(destWidth, result.getType().isa<UIntType>());


Do invalid's work with reset/clock/arrays and other types? I am pretty sure it works with aggregates, so you might need to check this (and just return {})

getExtendedConstant is intended to be used only for IntType. I'll add an assert to make this assumption more visible.

lattner · 2021-11-30T21:59:46Z

I agree with @seldridge here that more folding is probably ok. That said, I don't think it is important to invest effort in being super aggressive either.

Do we have a good list of things that really should be preserved with invalid? Documenting that will help put guard rails on this and set expectations. I think the model is pretty simple:

Never fold invalid values to zero through connects.
When lowering looks at them.
They are otherwise treated as zeros.

Does this sound right to you?

seldridge · 2021-12-01T01:36:06Z

Do we have a good list of things that really should be preserved with invalid?

Not exactly other than:

A when block with a default initialization of invalid should lower to "validif" which we represent as mux(cond, true, invalid). This has to be preserved and should be preserved by following your "Never fold invalid values to zero through connects". After expand whens, an invalid value connection can be converted to a connection to zero value.
A register initialized to an invalidated wire, port, or intstance port (through any number of connects within the same module) will have its reset/initialization removed.
Anything that doesn't violate (1) or (2) should be treated as constant zero.

This roughly aligns with what you're saying, but has a lot of pesky properties.

A byproduct of this is that the FIRRTL to HW conversion should see no invalid values if optimizations are on.

lattner · 2021-12-01T20:48:03Z

sounds good to me

fabianschuiki · 2021-12-03T08:33:28Z

I'll do another pass over this with the findings from the discussion here and elsewhere regarding invalid propagation.

fabianschuiki · 2021-12-03T13:11:00Z

@lattner, @seldridge, I've reworked this and extended it to cover all the prim ops. I've also added a bunch of tests for the newly-covered cases, and was able to get rid of @seldridge's XFAIL tests entirely (with a caveat that MFC is now more aggressive in some cases than SFC).

lattner · 2021-12-03T16:23:23Z

This looks structurally fine to me, but i'd prefer @seldridge to do a detailed walkthrough

seldridge

This looks awesome. I love how everything unifies around isConstant.

I will open issues about all of this on the SFC. The only ones that I'm slightly nervous about is places where we use specialconstant for clock and reset literals. The SFC doesn't have these, so there will always be asClock or asAsyncReset casts that it can't remove. (This is a place where these ops would block register removal.) This isn't enough to block these until we have a problem, thouhg.

include/circt/Dialect/FIRRTL/FIRRTLCanonicalization.td

lib/Dialect/FIRRTL/FIRRTLFolds.cpp

seldridge · 2021-12-03T17:17:45Z

lib/Dialect/FIRRTL/FIRRTLFolds.cpp

+    if (auto type = attr.getType().dyn_cast<IntType>())
+      return APSInt(type.getWidth().getValueOr(1), type.isUnsigned());


Can getWidth().getValue() fail here? I assume this is catching the case of this running on an uninferred width?

Yeah exactly. My thinking was that there's this getAPSInt() call below if we're dealing with an IntegerAttr, which would have a width assigned even in the case that the FIRRTLType itself had an uninferred width (just some random width that is sufficient to hold the constant). So in analogy I just made the invalidvalue fold to a zero of at least one bit.

lib/Dialect/FIRRTL/FIRRTLFolds.cpp

seldridge · 2021-12-03T17:32:19Z

lib/Dialect/FIRRTL/FIRRTLFolds.cpp

+      TypeSwitch<Operation *>(defOp)
+          .Case<ConstantOp, SpecialConstantOp, InvalidValueOp>(
+              [&](auto op) { attr = op.fold({}).template get<Attribute>(); });


MSVC may not like the template inside a lambda. You may want to do a special run with the MSVC action using this branch to make sure it compiles. If this is a problem, I will usually work around it by removing the auto. In this case it's necessary as there's no parent type that can be used here. Worst case, this could use three cases with explicit types instead of auto.

Good idea, will do a run!

lib/Dialect/FIRRTL/FIRRTLFolds.cpp

seldridge · 2021-12-03T17:39:01Z

lib/Dialect/FIRRTL/FIRRTLFolds.cpp

-  // rem(invalid, x) -> 0
+  /// rem(x, x) -> 0
+  ///
+  /// Division by zero is undefined in the FIRRTL specification.  This fold
+  /// exploits that fact to optimize self division remainder to zero.  Note:
+  /// this should supersede any division with invalid or zero.  Remainder of
+  /// division of invalid by invalid should be zero.
+  if (lhs() == rhs())
+    return getIntZerosAttr(getType());
+


This references division? Do we actually do this fold for remainder, too?

Yeah, there's a div(x, x) -> 1 and a rem(x, x) -> 0) fold which cater to the same idea. This is one of the things that aren't there in SFC, and it would be an easy fix to just disable the folds here to make MFC match up with SFC.

seldridge · 2021-12-03T17:40:51Z

lib/Dialect/FIRRTL/FIRRTLFolds.cpp

-    return lhs();
-
-  if (auto rhsCst = operands[1].dyn_cast_or_null<IntegerAttr>()) {
+  if (auto rhsCst = getConstant(operands[1])) {


This now only has to check the RHS because or(invalid, x) is properly canonicalized to or(x, invalid) because invalid is zero-like?

Yeah exactly. The OrPrimOp is marked as commutative, and the InvalidValueOp folds to a InvalidValueAttr (which I guess MLIR interprets as "the constant"), so a builtin pattern will swap the operands on that OrPrimOp.

seldridge · 2021-12-03T18:09:22Z

Tracking issue on the SFC about the observed differences: chipsalliance/firrtl#2432

youngar · 2021-12-04T02:28:06Z

There is a bug in HEAD compiling regression/test3 that this might fix, GetZeroConstant seems to always return a unsigned constant. Smaller test case here:

firrtl.circuit "Y" {
firrtl.module @Y(in %i: !firrtl.sint<1>, out %o: !firrtl.sint<15>) {
  %invalid_si1 = firrtl.invalidvalue : !firrtl.sint<1>
  %0 = firrtl.pad %invalid_si1, 15 : (!firrtl.sint<1>) -> !firrtl.sint<15>
  firrtl.connect %o, %0 : !firrtl.sint<15>, !firrtl.sint<15>
}
}

Any chance you could check if this is working now?

Edit: Yeah, this does fix both the test case above and test3 🥳

This extends the FIRRTL fold and canonicalization patterns to treat `invalidvalue` as a zero if it is an input to a primary operation. This is inline with what the Scala implementation of the FIRRTL compiler does. Doing this removes quite a few of the special cases where certain ops are aware of `invalidvalue`s and will fold as if it was a zero. The addition of the `getConstant`, `isConstantZero`, and `canonicalizePrimOp` helpers allow for most folders and canonicalizers to not having to bother with `invalidvalue`s, as these helpers implicitly map `invalidvalue`s to zero constants. This also subsumes the work done in llvm#2278 and llvm#2198, which selectively added handling for `invalidvalue`s to the `PadPrimOp`. The extension of the folders and canonicalizers now makes all the cases in `invalid-reg-fail.fir` pass, albeit some only after accepting that the MLIR implementation folds *additional* cases where the Scala one might just leave an operation untouched. These are marked with a `<-- fixed; upstream to Scala FIRRTL impl?` and should probably be upstreamed as additional canonicalizations on the Scala side as well.

fabianschuiki · 2021-12-04T08:21:48Z

Thanks for checking @youngar. I've added your test case as a regression test to canonicalization.mlir 👍.

fabianschuiki added the FIRRTL Involving the `firrtl` dialect label Nov 29, 2021

fabianschuiki requested review from seldridge and lattner November 29, 2021 11:50

fabianschuiki force-pushed the canonicalize-invalids branch from d240ffa to 87a6917 Compare November 29, 2021 11:51

uenoku mentioned this pull request Nov 29, 2021

[FIRRTL/IMConstantProp] Add new state "ValidValue" #2246

Closed

seldridge reviewed Nov 29, 2021

View reviewed changes

lattner reviewed Nov 29, 2021

View reviewed changes

seldridge mentioned this pull request Dec 2, 2021

[FIRRTL] Cast Folders for Invalid #2203

Closed

fabianschuiki force-pushed the canonicalize-invalids branch from 87a6917 to 760d069 Compare December 3, 2021 13:09

fabianschuiki changed the title ~~WIP: [FIRRTL] Improve canonicalization involving invalidvalue~~ [FIRRTL] Improve canonicalization involving invalidvalue Dec 3, 2021

fabianschuiki marked this pull request as ready for review December 3, 2021 13:09

fabianschuiki requested review from seldridge and lattner December 3, 2021 13:09

seldridge approved these changes Dec 3, 2021

View reviewed changes

fabianschuiki force-pushed the canonicalize-invalids branch from 760d069 to b8072b2 Compare December 3, 2021 20:10

fabianschuiki force-pushed the canonicalize-invalids branch from b8072b2 to b8053cc Compare December 4, 2021 08:21

fabianschuiki merged commit c351dc6 into llvm:main Dec 4, 2021

fabianschuiki deleted the canonicalize-invalids branch December 4, 2021 08:22

seldridge mentioned this pull request Dec 5, 2021

[FIRRTL] Sub Folder Can't Use AsUInt Before Pad #2289

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIRRTL] Improve canonicalization involving `invalidvalue` #2251

[FIRRTL] Improve canonicalization involving `invalidvalue` #2251

fabianschuiki commented Nov 29, 2021 •

edited

Loading

seldridge left a comment

fabianschuiki commented Nov 29, 2021

lattner left a comment

lattner Nov 29, 2021

fabianschuiki Dec 3, 2021

lattner commented Nov 30, 2021

seldridge commented Dec 1, 2021 •

edited

Loading

lattner commented Dec 1, 2021

fabianschuiki commented Dec 3, 2021

fabianschuiki commented Dec 3, 2021

lattner commented Dec 3, 2021

seldridge left a comment

seldridge Dec 3, 2021

fabianschuiki Dec 3, 2021

seldridge Dec 3, 2021

fabianschuiki Dec 3, 2021

seldridge Dec 3, 2021

fabianschuiki Dec 3, 2021

seldridge Dec 3, 2021

fabianschuiki Dec 3, 2021

seldridge commented Dec 3, 2021

youngar commented Dec 4, 2021 •

edited

Loading

fabianschuiki commented Dec 4, 2021

		if (auto type = attr.getType().dyn_cast<IntType>())
		return APSInt(type.getWidth().getValueOr(1), type.isUnsigned());

[FIRRTL] Improve canonicalization involving invalidvalue #2251

[FIRRTL] Improve canonicalization involving invalidvalue #2251

Conversation

fabianschuiki commented Nov 29, 2021 • edited Loading

seldridge left a comment

Choose a reason for hiding this comment

fabianschuiki commented Nov 29, 2021

lattner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lattner commented Nov 30, 2021

seldridge commented Dec 1, 2021 • edited Loading

lattner commented Dec 1, 2021

fabianschuiki commented Dec 3, 2021

fabianschuiki commented Dec 3, 2021

lattner commented Dec 3, 2021

seldridge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seldridge commented Dec 3, 2021

youngar commented Dec 4, 2021 • edited Loading

fabianschuiki commented Dec 4, 2021

[FIRRTL] Improve canonicalization involving `invalidvalue` #2251

[FIRRTL] Improve canonicalization involving `invalidvalue` #2251

fabianschuiki commented Nov 29, 2021 •

edited

Loading

seldridge commented Dec 1, 2021 •

edited

Loading

youngar commented Dec 4, 2021 •

edited

Loading