Ensure InlineCasts does not inline complex Expressions

[jackkoenig]

Previously, InlineCasts could inline complex (ie. non-cast) Expressions
into other complex Expressions. Now it will only inline so long as there
no more than 1 complex Expression in the current nested Expression.

This fixes the bug that #2128 is hitting in CI (the change in #2128 exposes this bug).

These tests probably aren't backportable but the bugfix should be, this is a functional bug.
First test is the functional bug, second is because my original fix caused some Verilog differences, namely the clock in that test would be named _T_2 which is obviously not what we want. I think code gen would benefit from some "better name" handling that works across casts. Regardless, this PR is a minor bugfix so I wanted it to not cause Verilog diffs if possible, and it doesn't.

The diff looks small but most of the diff is because I added an inner method to onExpr to propagate seenComplexExpr. I didn't really want to expose that on the public API.

Contributor Checklist

• Did you add Scaladoc to every public function/method?
• [NA] Did you update the FIRRTL spec to include every new feature/behavior?
• Did you add at least one test demonstrating the PR?
• Did you delete any extraneous printlns/debugging code?
• Did you specify the type of improvement?
• Did you state the API impact?
• Did you specify the code generation impact?
• Did you request a desired merge strategy?
• Did you add text to be included in the Release Notes for this change?

Type of Improvement

• bug fix

API Impact

No impact

Backend Code Generation Impact

No impact

Desired Merge Strategy

• Squash

Release Notes

Fix bug where expression inlining could emit Verilog that doesn't match the input FIRRTL's semantics

Reviewer Checklist (only modified by reviewer)

• Did you add the appropriate labels?
• Did you mark the proper milestone (1.2.x, 1.3.0, 1.4.0) ?
• Did you review?
• Did you check whether all relevant Contributor checkboxes have been checked?
• Did you mark as Please Merge?

[albert-magyar]

I think some Scaladoc/renaming is definitely higher priority than normal here; I wouldn't have imagined that InlineCasts inlined things other than casts.

Ultimately, this transform is a lot broader than inlining casts. It takes advantage of the fact that casts are no-ops in the Verilog emitter since all SIntType expressions get wrapped in $signed, and perhaps a better way to describe it would be stating that it greedily attempts to build larger expression trees that contain at most one expression AST node that is neither a cast nor a reference-like node. I know that renaming is a pain, so we don't necessarily have to do it soon, but I think InlineAcrossCasts might make it clearer that the transform inlines things other than casts.

[jackkoenig]

I think some Scaladoc/renaming is definitely higher priority than normal here; I wouldn't have imagined that InlineCasts inlined things other than casts.

Ultimately, this transform is a lot broader than inlining casts. It takes advantage of the fact that casts are no-ops in the Verilog emitter since all SIntType expressions get wrapped in $signed, and perhaps a better way to describe it would be stating that it greedily attempts to build larger expression trees that contain at most one expression AST node that is neither a cast nor a reference-like node. I know that renaming is a pain, so we don't necessarily have to do it soon, but I think InlineAcrossCasts might make it clearer that the transform inlines things other than casts.

I agree with all of this. I'll deprecate and rename in a follow on PR because I want this to backport to 1.2.x.

[albert-magyar]

This is a strict improvement over the existing pass so LGTM