[Arc] Infer StateOp reset from CompReg #4997
Conversation
| arc.getClockMutable().assign(clock); | ||
| arc.setLatency(arc.getLatency() + 1); | ||
| if (reset) | ||
| arc.getResetMutable().assign(reset); |
There was a problem hiding this comment.
What if there is already a reset present?
There was a problem hiding this comment.
Are you happy for this just to be covered with an assertion? I suppose we could comb.or multiple reset lines, but I'm unable to think of a situation where you would expect/desire a reset to already be present on the StateOp at this point, unless there are additions you have planned.
There was a problem hiding this comment.
The situation/assertion could be triggered with an input that has both state ops and compreg ops in there which does not happen currently. But the fact that a user can construct such an IR is enough of a reason that we shouldn't use an assertion. However, I think it's not necessary to support this case right now, so I would just go for an emitError in that case as well.
There was a problem hiding this comment.
Ah of course! Sounds good, I'll do that :)
TaoBi22
force-pushed
the
reset_from_compreg
branch
from
5c2043d
to
dbd7031
Compare
| builder.create<StateOp>(loc, defOp, std::get<0>(clockAndResetAndOp), | ||
| /*enable=*/Value{}, 1, inputs); | ||
| auto reset = std::get<1>(clockAndResetAndOp); | ||
| if (reset) | ||
| arcOp.getResetMutable().assign(reset); |
There was a problem hiding this comment.
Good catch it was actually passed as the enable here! Might be worth adding another builder to ArcOps.td for this case.
| @@ -0,0 +1,30 @@ | |||
| // RUN: arcilator %s --inline=0 --until-before=state-lowering | FileCheck %s | |||
There was a problem hiding this comment.
I'm wondering why this is added as an integration test. Couldn't we just add it do the existing regression tests for ConvertToArcs?
| // CHECK-DAG: arc.output [[ARG0]] | ||
| // CHECK-NEXT: } | ||
|
|
||
| // CHECK-DAG: hw.module @Trivial([[CLOCK:%.+]]: i1, [[I0:%.+]]: i4, [[RESET:%.+]]: i1) |
There was a problem hiding this comment.
I think it'd be better to use CHECK-LABEL here and in general only use CHECK-DAG where it really makes sense/is necessary.
There was a problem hiding this comment.
I've hopefully addressed this now, but I'm not particularly well acquainted with FileCheck & its preferred usage so feedback is appreciated!
| // CHECK-DAG: [[RES2:%.+]] = arc.state @[[ARC]]([[I0_2]]) clock [[CLOCK]] reset [[RESET1]] lat 1 {names = ["foo"] | ||
| // CHECK-DAG: [[RES3:%.+]] = arc.state @[[ARC]]([[I0_2]]) clock [[CLOCK]] reset [[RESET2]] lat 1 {names = ["bar"] |
There was a problem hiding this comment.
For example, I think here it's fine to use CHECK-DAG, but not strictly necessary.
Co-authored-by: Martin Erhart <maerhart@outlook.com>
Update the
ConvertToArcspass so that if aCompReghas an explicit reset operand, this becomes theStateOp's reset operand. SinceStateOps currently can only reset to 0, the pass fails if anyCompReg's reset value is not a constant 0.