[FIRRTL] Add folders for add and mul

[mtsokol]

Hi @mikeurbach,

This PR addresses #6724 and #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]

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.

[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.