[Ibis] Add instance/port references and related ops #5773
Conversation
Since port reads/writes are now done at a classref-based level, this also means that there's no reason to have specific instance reads/writes. A bit of an outstanding question - `ibis.container` was isolated from above, which i still think is the right descision. However, we need to be able to grab `%this` somehow... Some options: 1. Add a block argument to containers, e.g. `ibis.container @d(%parent)` wherein %parent is implied to be %this of the class 2. Disallow IsolatedFromAbove (what's currently done).
| @@ -21,6 +21,9 @@ def PortOpInterface : OpInterface<"PortOpInterface"> { | |||
| [{"An interface for operations which describe ports."}]; | |||
|
|
|||
| let methods = [ | |||
| InterfaceMethod< | |||
| [{"Returns the data type of the port."}], | |||
There was a problem hiding this comment.
Uber nit: for short strings, just use "". This'll include the "" in the string I think.
| returned `ibis.classref` will be an opaque type. | ||
| }]; | ||
|
|
||
| let arguments = (ins AnyClassRefType:$src); |
There was a problem hiding this comment.
To support getting the grand parent of an instance, the argument must accept either type.
| ]; | ||
| } | ||
|
|
||
| def AnyClassRefType : Type< |
There was a problem hiding this comment.
nit: "any" could be interpreted as including unknown classes or as being the Opaque version (as I first figured it was before reading the below). Might I suggest something like "known" or "transparent" or something to that effect?
There was a problem hiding this comment.
What do you mean by unknown classes? By AnyClassRefType i just want to capture "either !ibis.classref or !ibis.classref<T>"
| }]; | ||
|
|
||
| let arguments = (ins | ||
| AnyClassRefType:$src, |
There was a problem hiding this comment.
You don't know the class for the first get_child.
| }]; | ||
| } | ||
|
|
||
| def GetChildOp : IbisOp<"get_child"> { |
There was a problem hiding this comment.
"child" is somewhat ambiguous in this case. May use the term "instance" which is more precise.
There was a problem hiding this comment.
Attempted to disambiguate.
test/Dialect/Ibis/round-trip.mlir
Outdated
| // CHECK-NEXT: %2 = ibis.port.read @C_out : i1 | ||
| // CHECK-NEXT: ibis.instance.port.write @a::@A_in(%2) : i1 | ||
| // CHECK-NEXT: %3 = ibis.instance.port.read @a::@A_out : i1 | ||
| // CHECK-NEXT: %3 = ibis.get_port %this, @C_in : !ibis.classref<@C> -> !ibis.portref<i1> |
There was a problem hiding this comment.
Plz also add tests for getting the port of a parent and getting a port from a cousin.
There was a problem hiding this comment.
In this state, there isn't a method to get a port from a parent (which i also think we discussed during the last meeting, that we only need to access a sibling through the parent). Although i'd expect us to probably also need to acquire parent ports as well. This is an issue seeing as currently get_port requires a typed classref to extract a port. We could obviously make it so that opaque classrefs are accepted for get_port and then do no validation there either.
| // PREP: ibis.method @getAndSetWrapper(%arg: !hw.struct<new_value: ui32>) -> ui32 { | ||
| // PREP: %new_value = hw.struct_explode %arg : !hw.struct<new_value: ui32> | ||
| // PREP: %0 = hw.struct_create (%new_value) {sv.namehint = "getAndSet_args_called_from_getAndSetWrapper"} : !hw.struct<x: ui32> | ||
| // PREP: %1 = ibis.call @c::@getAndSet(%0) : (!hw.struct<x: ui32>) -> ui32 | ||
| // PREP: %1 = hw.struct_create (%new_value) {sv.namehint = "getAndSet_args_called_from_getAndSetWrapper"} : !hw.struct<x: ui32> |
There was a problem hiding this comment.
I don't like including namehints in tests but it's a judgement call.
There was a problem hiding this comment.
I personally like to keep round-trip tests verbatim what the output IR is.
| def GetParentOp : IbisOp<"get_parent"> { | ||
| let summary = "Ibis get parent"; | ||
| let description = [{ | ||
| Given an Ibis class reference, returns the parent of said class. The |
There was a problem hiding this comment.
Should the description say what happens if there is no parent?
There was a problem hiding this comment.
I can add it - as with most things parent related it'll be "won't be detectable until some pass which looks more closely at the instance hierarchy resolves parents". There might also be merit in having an explicit instance hierarchy verification pass which opt-in validates this (i.e. not part of the op verifiers).
| let summary = "Ibis get instance"; | ||
| let description = [{ | ||
| Given an Ibis class reference, returns a instance of said class. The instance | ||
| is specified by the symbol name of the instance of that instance in the referenced |
There was a problem hiding this comment.
Typo, should it be "symbol name of the instance in the instance in the referenced class."?
Since port reads/writes are now done at a classref-based level, this also means that there's no reason to have specific instance reads/writes.
A bit of an outstanding question -
ibis.containerwas isolated from above, which i still think is the right descision. However, we need to be able to grab%thissomehow...Some options:
ibis.container @D(%parent)wherein %parent is implied to be %this of the class