[FIRRTL] Extend emitConnect to open aggregates.#10017
[FIRRTL] Extend emitConnect to open aggregates.#10017
Conversation
Refactor aggregate handling to common helper.
seldridge
left a comment
seldridge
left a comment
There was a problem hiding this comment.
I don't exactly love the templating and I've been trying to think of a way out of it. I can't think of a good alternative that doesn't have a bunch of duplication.
| builder.restoreInsertionPoint(locBuilder.saveInsertionPoint()); | ||
| } | ||
|
|
||
| template <typename ATy, typename IndexOp, bool isBundle /* check flip? */> |
There was a problem hiding this comment.
Can the PR clarify what the inline comment means? I think this is saying that if isBundle is true, then it will check for flips. (I'm intentionally not reading the later code just yet.) If that is what it is, then maybe this should be checkFlip. WDYT?
After reading the rest of the code, I think isBundle is correct! Disregard.
| if constexpr (isBundle) { | ||
| if (dstAggTy.getElement(i).isFlip) | ||
| std::swap(dstField, srcField); | ||
| } |
There was a problem hiding this comment.
Nit:
| if constexpr (isBundle) { | |
| if (dstAggTy.getElement(i).isFlip) | |
| std::swap(dstField, srcField); | |
| } | |
| if constexpr (isBundle && dstAggTy.getElement(i).isFlip) | |
| std::swap(dstField, srcField); |
| builder, dst, dstFType, src, srcFType)) && | ||
| failed( | ||
| connectIfAggregates<OpenVectorType, OpenSubindexOp, false>( | ||
| builder, dst, dstFType, src, srcFType))) { |
There was a problem hiding this comment.
Kind of a nit, but I'm trying to see if there is a way to avoid templating here... The best that I can come up with is that switching the if/else checks to use TypeSwitch would give the PR the outer template parameter for free. Not blocking, just something to consider. This is also a larger change.
2 participants
Refactor aggregate handling to common helper.