[FIRRTL] Add a folder for add property op

[mtsokol]

Hi @mikeurbach,

This PR addresses #6696.

It's a first-time contribution for me and these issues remained open for quite some time. I hope I didn't take anyone's assignment.

In canonicalization.mlir file I got an error when tried to reformat firrtl.class @PropertyArithmetic... line into multiple lines. Does firrtl.class support same format as firrtl.module in mlir files?

In a follow-up PR I can add other folding rules that were listed in aforementioned issues. Does x + x -> x * 2 fold also makes sense here?

[mikeurbach]

Thank you for working on this!

Left some comments inline. A couple general points...

This should really be split into two PRs. There is a reason we have separate issues for the separate operations: we generally try to make changes as small as possible, and prefer to send many many small changes. This is spelled out in the LLVM developer policy, which we try to follow in CIRCT: https://llvm.org/docs/DeveloperPolicy.html. Specifically, the section on incremental development describes some of the rationale here. I would prefer to see two PRs for these changes, one for each issue.

Keep in mind that we also follow the LLVM model for code review: https://llvm.org/docs/CodeReview.html. At some point, once you get commit access, you have discretion to send "obvious" patches without pre-merge review. This is a policy that works well with incremental development.

[mikeurbach]

I think the result logic might be flipped between add and multiply... Don't we normally add bitwidths for multiplication and take max width + 1 for addition?

[mtsokol]

Ahh! 😅 Exactly, they're swapped. I took both from the FIRRTL spec but the other way around. It just shows that my test case is too trivial!

[mikeurbach]

Also, keep in mind that these are not the operations you linked to in the FIRRTL spec. Those are the primitive hardware operations for add and mul. What you are adding folders for is the primitive property operations for integer_add and integer_mul: https://github.com/chipsalliance/firrtl-spec/blob/main/spec.md#integer-add-operation

These are already just defined as "arbitrary precision". Do we need the explicit extension, or can we just use + or * with the APInts to get the right result (i.e., does APInt extend for us?).

[mtsokol]

My first attempt was to implement these operations without calling extend but unfortunately this results in an incorrect outcome. For example 2 and 5 (which are APSInts that wrap APInt) are passed with bit-width 3 and multiplying them directly doesn't extent the bit-width to 4 and the result is -6 instead of 10.

AFAIC APSInt class doesn't have an automatic bit-width adjustment mechanism. It can be done by calling extend or trunc and passing the new width explicitly only. One might wonder how %5 = firrtl.integer 5 knows the required number of bits. It's done by APSInt::APSInt(StringRef Str), the constructor - it overestimates the number of bits, then checks how many are actually used with getSignificantBits and truncs it.

So IMO we need an explicit extension, which isn't troubling as we know the formula for output's bit-width.

[mtsokol]

Also, I see that hardware ops, like add and mul, inherit from IntBinaryPrimOp which has pretty flexible inferType and does this inference there (e.g. for add it's impl::inferAddSubResult).

OTOH Property operations inherit from IntegerBinaryPrimOp where the inferType is hardcoded for all ops!

// We use a static inferType that always returns FIntegerType.
let inferType = "getFIntegerType";

// Define the getFIntegerType inline in ODS.
let firrtlExtraClassDeclaration = [{
  static FIntegerType getFIntegerType(FIRRTLType lhs,
                                      FIRRTLType rhs,
                                      std::optional<Location> loc) {
    return FIntegerType::get(lhs.getContext());
  }
}];

As you can see inferred type is just the type of LHS of the op.

[mikeurbach]

I see, yep thanks for digging into this more, the explicit extension makes sense to me.

[mikeurbach]

Any chance this could re-use this helper?

circt/lib/Dialect/FIRRTL/FIRRTLFolds.cpp

Lines 204 to 210 in f83436b

	/// Applies the constant folding function `calculate` to the given operands.
	///
	/// Sign or zero extends the operands appropriately to the bitwidth of the
	/// result type if \p useDstWidth is true, else to the larger of the two operand
	/// bit widths and depending on whether the operation is to be performed on
	/// signed or unsigned operands.
	static Attribute constFoldFIRRTLBinaryOp(

I assume not, because that helper wants to work on hardware types and we specifically made separate Property types for !firrtl.integer.

It might be worth coming up with a similar helper for Property... but I think it's fine to leave this as-is for the first two folders before trying to generalize. Eventually if we have 3+ folders with a lot of repetition, maybe we make a helper.

[mtsokol]

I looked into it and constFoldFIRRTLBinaryOp (and all hardware types' folders) are pretty similar but I can't use them right away.

As explained in the comment above, Property Ops inherit from IntegerBinaryPrimOp which hardcodes result type to the type of LHS (and also imposes the bitwidth of the result). constFoldFIRRTLBinaryOp calls op->getResult(0).getType() to obtain the result type so we can't use this helper for Property Ops.
(For hardware ops there are separate functions for inferring the result type).

For now I agree to keep it simple and infer directly in the folder, but what is your opinion on refactoring IntegerBinaryPrimOp (Property base class) so that inferType isn't hardcoded but rather overridable by ops? Then I think potentially Hardware ops' logic can be reused for Properties.

Is there a reason why inferType for Property Ops is hardcoded to just LHS type?

[mikeurbach]

I see that both these comment threads converged to the same inferType, so I'll comment about that aspect here...

I think this has been required since I first added IntegerBinaryPrimOp: #6701 (comment). So that's why it seems like inferType is kind of useless and hardcoded. Also, note that unlike the hardware types, FIntegerType does not carry the width in the type, so returning LHS or RHS is equivalent, and equivalent to constructing a fresh FIntegerType as well.

Anyway, I think there may be merit in considering if we can better integrate with the usual folding infrastructure for hardware types, but if we don't carry widths in FIntegerType, I don't really see how we could. And I don't think we necessarily want that either. So, what is in this PR makes sense to me. Thanks for considering my offhand question so deeply.

[mikeurbach]

nit: it made the diff more confusing to move these to the bottom and rename their SSA values. At first, I thought "where'd the shl folder tests go".

I do like the name better, but in this case I think we'd prefer a smaller diff. Specifically, I think a couple LLVM guidelines apply here:

https://llvm.org/docs/CodingStandards.html#introduction (the note about sticking to existing style)
https://llvm.org/docs/DeveloperPolicy.html#incremental-development (each change should be kept as small as possible)

In this case, it should be possible to update this test file by adding new outputs and operations, without changing existing lines besides the module signature.

[mtsokol]

Right, there's no need to enforce any particular order of these tests. Done! And I only renamed result values for shl.

[mtsokol]

@mikeurbach Thank you for all first-PR guidance! I reduced this PR to add folder only - it's ready from my side. The follow-ups will contain mul folder (and any infer-type refactorings if we decide so).

[mikeurbach]

Thanks for the iterations on this, this looks good to me!