Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support splitting up struct method parameters into multiple input ports #729
base: main
Are you sure you want to change the base?
Support splitting up struct method parameters into multiple input ports #729
Changes from 44 commits
49532f4
363cc7b
b24ca9c
92fa2e8
c6b22fa
57258ec
8fdb09d
5945a7c
25695a4
6568cc8
1b1f033
730f044
7975da7
3ba06d4
96c35e5
d08c97a
b651d36
4020ade
2c9e86e
0c15378
1ecce93
936d140
eccaf03
4e48c17
14fb02c
7aea56f
af1e1eb
a52f2e6
36a4dc2
8999a65
be3d910
150d29c
c8114b1
0ea992e
9d7921f
f0d8ac7
7067fbe
0e23ad0
1329534
6f8c504
b1165c9
0868d79
df0f863
d7b1859
82a1c3f
5a34e93
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume that
primMethod
here is only exported so that it can be used inPreludeBSV
forvMkRWire1
and can be removed when that's resolved.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, would the size of
(x,())
be reported as 1? Do we think it should be 2?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I suppose it would be reported as 1.
I don't think it's too unreasonable to treat tuples that end in
()
as one element smaller. Note thatAppendTuple
also would drop the()
if the first tuple being appended ends in()
, and the same forCurryN
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recall that there is a general issue with tuples: because they are build from nested pairs, the last element can't be of a tuple type, otherwise it's indistinguishable from one large tuple. For example (and this works in BSV too), if you have a variable of type
Tuple2 a (Tuple2 b c)
, you can assign to it withtuple3
-- and the current implementation ofTupleSize
considers it 3. Here's code that you can try:However, if we have a variable of type
Tuple3 a b ()
, that cannot be assigned withtuple2
-- BSC gives a type error unless you usetuple3
. HoweverTupleSize
says its 2. This inconsistency seems wrong to me.The issue is that the expression
(e1,e2)
parses intoPrimPair e1 e2
, and(e)
parses the same ase
, and()
parses intoPrimUnit
. Longer tuples are nestings ofPrimPair
, butPrimUnit
is not a zero ofPrimPair
-- this is perhaps clearer in BSV, where you can't write()
, it has to be written asvoid
.The inconsistency can be fixed for
TupleSize
by defining it like this:But you're saying that
AppendTuple
andcurryN
also usePrimUnit
as a zero. Here is an example showing that appending aTuple3
andTuple2
will giveTuple4
if theTuple3
ends withPrimUnit
:This seems to be because you want to support
()
as a zero in calls toappendTuple
andsplitTuple
, and so there need to be instances for()
, but you have also written your typeclass so that it reuses the top-level instances for sub-elements of the tuple. If you separate the processing of the sub-elements from the top-level, I think the issue goes away. Of course, there's also a complication that BSC won't allow overlapping instance of the formT () x
andT x ()
, even if a more explicit instance for the intersectionT () ()
is given -- which is unfortunate, and maybe something that can be fixed one day? You got around that by adding a second level of typeclass. However, that second level is still conflating top-level()
with sub-element()
. I think you just need to add a third level of typeclass, that's only for processing once top-level()
has been eliminated -- and in fact I confirmed that this works:I haven't looked closely at
curryN
, but I assume that a layer of hierarchy could be added, with()
not handled in the second layer.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was worried that there's still the issue that
splitTuple
ofTuple3 Bool Bool ()
to split off the last element and thenappendTuple
to put it back won't result in the original tuple. But actually, both definitions are allowed -- by your implementation and mine! Unless I'm missing some reason why this example compiles:And it's not because the types are considered the same, because adding
y1 == y2
causes a type error.There's a dependency on the typeclass, so for the same arguments, only one result type should be possible:
Separately, isn't there something missing in the dependencies you declared? Shouldn't it also have
a c -> b
andb c -> a
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just getting back to your comments here - I'm not sure I agree your version of
TupleSize
is what we want?The version you specified would have
TupleSize () 1
, but we want()
to have size0
for the purposes ofcheckPortNames
. I suppose we could introduceTupleSize'
and make(a, ())
have size 2 but()
have size 0 - would that address your concern?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the user writes a
conflict_free
attribute for two rules, then BSC will add logic to dynamically check that any conflicting methods between the two have not been called at the same time. BSC does this by instantiating anRWire
for each method called by each rule, and inserting awset
action at the place in the rule where it calls the method; then a $display is inserted into the module, which prints an error message if the wires for two conflicting methods is ever true.This occurs in BSC after scheduling, when the module is in
ASyntax
. To add a submodule instantiation at that point, BSC needs anAVInst
value. The reason thatvMkRWire
exists is so that BSC can get its hands on theAVInst
forRWire
. It does this by simulating the entire compilation flow forvMkRWire
-- first typecheck, then convert to ISyntax, then elaboration, then convert to ASyntax -- and then looking for the sole submodule in the resulting ASyntax.The interface for
vMkRWire
is entirely unneeded -- BSC just wants to get theAVInst
structure for the_rw
submodule instantiation. We might consider changing this module to have an empty interface, and then you don't need to worry about addingprimMethod
to any methods.The reason that you needed to add
primMethod
is because BSC's simulated compilation flow starts at typecheck, without anyGenWrap
stage. It used to be OK for BSC to skipGenWrap
, because later stages only required that the interface be raw types (bit and PrimAction). But you've changed the evaluator to expect that all methods evaluate toIMethod
, and that only happens when there's a call toprimMethod
, and that gets inserted by thetoWrapField
call thatGenWrap
now inserts on the interface. SoAAddSchedAssumps
would need to simulate theGenWrap
step by insertingtoWrapField
; but if that's not done, then you would need to manually insert thetoWrapField
, or manually write the result of that (by insertingprimMethod
on each method).I think it might be better to write
toWrapField
on_rw
, rather than explicitly spell out the interface; and better than that would be to updateAAddSchedAssump
to insert it; but better still would be to eliminate the interface onvMkRWire1
altogether -- but actually, I would prefer that we eliminate the simulated compile flow altogether and just hardcode anAVInst
value for RWire inAAddSchedAssump
.However, all of this does make me wonder: For imported modules in BH code, we have written them as an import of the raw interface and then a wrapper module (that converts from the raw interface and inserts calls to primitives to save types etc) -- for example,
mkReg
is a wrapper aroundvMkReg
. Can we simplify those wrappers now with just a call totoWrapField
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, it looks like someone added a
vMkUnsafeRWire1
version of this module, when they were creatingUnsafe
versions of all theRWire
modules, but there's no need for that, so I'd delete that module.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest I haven't dug into
AAddSchedAssump
suffieciently to understand what is going on there. Although constructing anAVInst
directly does seem a lot nicer than simulating the whole compilation flow, if doing so is reasonable.I did try changing the
primMethod
totoWrapField
, but that doesn't work becauseVRWireN.wset
returns aPrimAction
, buttoWrapField
usesActionValue_ 0
. The implementation ofActionValue_
is not exported from the Prelude, and even if it was, we can't manipulate the result oftoWrapField
since it is wrapped inprimMethod
. I suppose we could maketoWrapMethod
usePrimAction
instead ofActionValue_ 0
for wrappingActionValue
s? But this is likely to change again anyway in the future when we add support for methods with multiple output ports.I suppose it is possible to use
toWrapMethod
in the manually-written wrapper modules for imported Verilog. Although in those cases the wrapped interface field types need to be written down anyway. Also this would require fixing the above-mentionedPrimAction
/ActionValue_ 0
disparity.