Fix subaccess

[sequencer]

Contributor Checklist

• Did you add Scaladoc to every public function/method?
• 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

None

Backend Code Generation Impact

Bug fix for nested SubAccess, introduce one more And in firrtl.passes.RemoveAccesses, but should be eliminated in next optimization passes.
In nested SubAccess, fix the bug in #1959

Desired Merge Strategy

• Rebase: You will rebase the PR onto master and it will be merged with a merge commit.

Release Notes

Bug fix for nested SubAccesses

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?

[ekiwi]

Have you considered creating a node for the index expression?
I am just wondering, whether it would make for better firrtl and for a simpler compiler if we would generate nodes for all non-trivial index expressions and then visit them.

[sequencer]

I got your point. That would be much better and reasonable to give most of FIRRTL generated expression with a readable name. It will much benefit generated verilog.
Just peek the original implementation before go to bed, I think there is still lacking of a infrastructure to get context for an Expression.
For example, most functions in a transform are directly manipulating a small FirrtlNode, which don't know where it is, and thus it still lack of enough information to construct its desired name for creating a DefNode.
I think we can have a global context cache, something like CircuitGraph. Or give every FirrtlNode a parents and children to reflect its own location in the AST, and add another pass to analyse AST, if we need to use the hierarchy of AST tree.

[ekiwi]

That would be much better and reasonable to give most of FIRRTL generated expression with a readable name.

I wasn't too concerned about readable names, for now, adding a new _GEN_... node should be fine.

[sequencer]

I see, but what benefits can be provided, if this node have no readable name?

[azidar]

I think this expression could be cached, but IMO this is such a rare occurrence I don't think its too bad.