[FIRRTL] Const handling in cast ops #5151

trilorez · 2023-05-08T20:54:37Z

This commit adds a ConstCastOp for casting away constness from types. This op and const types will be dropped early in the firrtl pipeline in a followup commit.

Verification has been added to other casting ops to ensure that type constness is taken into account.

dtzSiFive

Generally LGTM, added some comments/questions.

Does const work w/as{UInt,SInt,Clock,...} (just curious: how)? At a glance don't see it mentioned in other PR's, but they do feel like "cast" operations.

Will look at other PR's to put this in context, thanks I think that helps a lot.

include/circt/Dialect/FIRRTL/FIRRTLExpressions.td

dtzSiFive · 2023-05-10T13:57:04Z

lib/Dialect/FIRRTL/FIRRTLOps.cpp

-    if (*inTypeBits == *resTypeBits)
+    if (*inTypeBits == *resTypeBits) {
+      // non-'const' cannot be casted to 'const'
+      if (containsConst(getType()) && !isConst(getOperand().getType()))


The rule here is that bitcasts where the destination type has const anywhere /must/ originate from a fully-const input (even if they're the same type), is that intended/right? Is there a way to go from a bundle<x: const.uint<1>> to bundle<x: const.sint<1>> (or const.uint<1> to const.sint<1> for that matter?).

Checking if the bitcast portions are const-compatible sounds like a pain so not really suggesting that, but consider adding a test for the behavior or otherwise make that clear.

As an aside, the difference between a "const" type and a type that has const isn't always clear (which seems context-dependent), if there is phrasing that helps here that'd be great, especially for user-facing diagnostics.

An aggregate with all elements "const" being different than a const aggregate feels a little strange (as does const.bundle<x : const.uint<1>>) but I know that whole deal went through iterations and not sure how that could best be changed so there are canonical representations (e.g., outer const implies inner const).

I'm not sure how important this is (might be but might very much not be), doesn't seem like we use bitcast a lot?

The idea is mainly to disallow non-const to const as a basic sanity-check without doing the full const-compatible check, since a const-cast would be more appropriate.

But on the other hand, bitcasts seem to be purely internal as opposed to an actual firrtl op and aren't introduced until the lower-types pass, which is after where the drop-const pass will be, so maybe I don't need to do anything for this op?

dtzSiFive · 2023-05-10T13:57:13Z

lib/Dialect/FIRRTL/FIRRTLOps.cpp

+      .Default(false);
+}
+
+LogicalResult ConstCastOp::verify() {


Thanks for adding the verifier!

lib/Dialect/FIRRTL/FIRRTLTypes.cpp

dtzSiFive · 2023-05-10T14:11:56Z

lib/Dialect/FIRRTL/FIRRTLTypes.cpp

+      auto destElement = destElements[i];
+      auto srcElement = srcElements[i];
+      if (destElement.name != srcElement.name ||
+          destElement.isFlip != srcElement.isFlip ||


Not a big deal but was curious: apparently can cast const.bundle<a: T, const b: U> to const.bundle<const a: T, b: U> which seems harmless since AFAIK the two are basically equivalent anyway (?).

It's weird but I consider it to be ok.

lib/Dialect/FIRRTL/FIRRTLTypes.cpp

trilorez · 2023-05-10T15:38:54Z

Thanks for the review!

Does const work w/as{UInt,SInt,Clock,...} (just curious: how)? At a glance don't see it mentioned in other PR's, but they do feel like "cast" operations.

The "as..." ops are handled in #5153 since they are prim ops. There, the ops preserve constness. See example.

youngar · 2023-05-17T04:30:12Z

lib/Dialect/FIRRTL/FIRRTLTypes.cpp

+  bool srcIsConst = srcType.isConst() || srcOuterTypeIsConst;
+
+  // Cannot cast non-'const' src to 'const' dest
+  if (destType.isConst() && !srcIsConst)
+    return false;


Maybe move all the ground type handling down to the bottom like you did in the areTypesEquivalent PR.

youngar · 2023-05-17T04:54:02Z

I am trying to think of some weird cases where flip matters, I think I have one but it might not even matter. I guess a big question I have is, when are we going to end up using this operation? Is this for early lowering of connects to strict connects?

Here is the rough example:

firrtl.module(in %in : !firrtl.bundle<a flip: uint<1>>, out %out : !firrtl.bundle<a flip: const.uint<1>>) {
  // Option 0: don't use strict connect:
  firrtl.connect %out, %in : !firrtl.bundle<a flip: const.uint<1>>, !firrtl.bundle<a flip: uint<1>>
  
  // Option 1:  const-cast has the same "flow" as its input, meaning that it creates an alias to the input value.
  // This allows us to drive directly to the result of the cast:
  %0 = firrtl.constCast %out : (!firrtl.bundle<a flip: const.uint<1>>) -> !firrtl.bundle<a flip: uint<1>>
  firrtl.strictconnect %0, %in :  !firrtl.bundle<a flip: uint<1>>
  
  // Option 2: const-cast can add "const" to anything with a flip:
  %0 = firrtl.constCast %in : (!firrtl.bundle<a flip: uint<1>>) -> !firrtl.bundle<a flip: const.uint<1>>
  firrtl.connect %out, %0 : !firrtl.bundle<a flip: const.uint<1>>
}

edit: it appears to be important over here: https://github.com/llvm/circt/pull/5153/files#diff-c8689caabdf068960b794c6990c63337929f28b354cf839c2a43abb2973c23a6R126-R130

So it seems like we might fail to const-cast the two, and then hit the assert below?

youngar · 2023-05-17T05:00:12Z

include/circt/Dialect/FIRRTL/FIRRTLExpressions.td

+  : PredOpTrait<"operand constness must match",
+                 CPred<"isConst($" # a # ".getType()) == isConst($" # b # ".getType())">>;
+
+def UninferredResetCastOp : FIRRTLOp<"resetCast", [HasCustomSSAName, Pure, SameGroundTypeOperandConstness<"input", "result">]> {


We do try to keep the tablegen files to 80 (I think) columns.

youngar · 2023-05-17T05:00:14Z

include/circt/Dialect/FIRRTL/FIRRTLExpressions.td

@@ -938,6 +943,17 @@ def UninferredWidthCastOp : FIRRTLOp<"widthCast", [HasCustomSSAName, Pure]> {
    "$input attr-dict `:` functional-type($input, $result)";
 }

+def ConstCastOp : FIRRTLOp<"constCast", [HasCustomSSAName, Pure]> {
+  let description = [{
+    Cast from a 'const' to a non-'const' type.


I like to add an example just demonstrating the op syntax. It makes the website documentation a little bit more useful.

Example added.

youngar · 2023-05-17T05:14:04Z

include/circt/Dialect/FIRRTL/FIRRTLOps.h

+/// Returns true if the type is or contains a 'const' type.
+bool containsConst(Type type);
+


It seems like it should go in FIRRTLTypes.h or FIRRTLUtils.h.

Yeah, not sure why I put it here in the first place. Moved along with isConst to FIRRTLTypes.h. Also updated the documentation to better reflect the spec.

youngar

A couple nits

trilorez · 2023-05-17T16:57:18Z

So it seems like we might fail to const-cast the two, and then hit the assert below?

I think emitConnect handles this by swapping flipped elements. It looks like in that parsing branch, I can add a const-castable check here though to avoid doing unnecessary element-wise connects.

Relatedly, I'm thinking I should enforce that only passive types are const-castable to keep the logic simpler. Based on the emitConnect, that should handle the use cases for the op.

trilorez · 2023-05-17T17:24:46Z

Updated to require passive types for the cast op.

youngar · 2023-05-17T17:24:57Z

Relatedly, I'm thinking I should enforce that only passive types are const-castable to keep the logic simpler. Based on the emitConnect, that should handle the use cases for the op.

Sure, the more powerful the cast op is, the less connect operations we will need, its just a trade-off. Since we are breaking down connects eventually anyways, you might as well only allow ground types to be const-cast :P. But it looks like enforcing a passive operand will make the current code work with no changes.

youngar · 2023-05-17T17:26:52Z

lib/Dialect/FIRRTL/FIRRTLTypes.cpp

+    return llvm::all_of_zip(destElements, srcElements,
+                            [&](const BundleType::BundleElement &destElement,
+                                const BundleType::BundleElement &srcElement) {
+                              return destElement.name == srcElement.name &&
+                                     destElement.isFlip == srcElement.isFlip &&
+                                     areTypesConstCastable(destElement.type,
+                                                           srcElement.type,
+                                                           srcIsConst);
+                            });


Should this check !destElement.isFlip && !srcElement.isFlip, i.e., that the bundle is passive?

If this is only called by the ConstCastOp verifier (I believe the plan is that it is not), then this will have already been checked.

Good point. I added a passive check above so every element doesn't need to check flip.

This commit adds a ConstCastOp for casting away constness from types. This op and const types will be dropped early in the firrtl pipeline in a followup commit. Verification has been added to other casting ops to ensure that type constness is taken into account.

…peOperandConstness

youngar

LGTM thanks!

youngar · 2023-05-17T19:10:42Z

lib/Dialect/FIRRTL/FIRRTLTypes.cpp

+    auto destElements = destBundleType.getElements();
+    auto srcElements = srcBundleType.getElements();
+    size_t numDestElements = destElements.size();
+    if (numDestElements != srcElements.size())
+      return false;
+
+    return llvm::all_of_zip(destElements, srcElements,
+                            [&](const BundleType::BundleElement &destElement,
+                                const BundleType::BundleElement &srcElement) {
+                              return destElement.name == srcElement.name &&
+                                     areTypesConstCastable(destElement.type,
+                                                           srcElement.type,
+                                                           srcIsConst);
+                            });


Might try inlining the locals or const auto & for arguments and see if the auto-formatter picks a different layout. I really don't love when it indents the body of the lambda so far. (If you like it like this its totally fine).

Seems slightly better with const auto &

trilorez requested review from darthscsi and dtzSiFive May 8, 2023 20:54

trilorez mentioned this pull request May 8, 2023

[FIRRTL] DropConst pass #5152

Merged

trilorez force-pushed the dev/trilorez/const-cast branch 2 times, most recently from 4941449 to 54c0555 Compare May 9, 2023 02:22

dtzSiFive reviewed May 10, 2023

View reviewed changes

trilorez force-pushed the dev/trilorez/const-cast branch from 084edfe to 029a445 Compare May 15, 2023 17:48

trilorez requested a review from youngar May 16, 2023 20:29

youngar reviewed May 17, 2023

View reviewed changes

trilorez force-pushed the dev/trilorez/const-cast branch from 029a445 to 236befe Compare May 17, 2023 17:24

youngar reviewed May 17, 2023

View reviewed changes

trilorez added 4 commits May 17, 2023 12:49

Update const castable check based on pr feedback, rename SameGroundTy…

a80704d

…peOperandConstness

PR updates

cc0b11e

Add passive check

3e54136

trilorez force-pushed the dev/trilorez/const-cast branch from 313216c to 3e54136 Compare May 17, 2023 18:53

youngar approved these changes May 17, 2023

View reviewed changes

Improve lambda formatting

8c11b4d

trilorez merged commit 2423baa into main May 17, 2023

trilorez deleted the dev/trilorez/const-cast branch May 17, 2023 19:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[FIRRTL] Const handling in cast ops #5151

[FIRRTL] Const handling in cast ops #5151

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

		/// Returns true if the type is or contains a 'const' type.
		bool containsConst(Type type);

[FIRRTL] Const handling in cast ops #5151

[FIRRTL] Const handling in cast ops #5151

Uh oh!

Conversation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!