Skip to content
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

[Ibis] Add container and port operations #5747

Merged
merged 5 commits into from
Aug 3, 2023
Merged

Conversation

mortbopet
Copy link
Contributor

Adds: container, container instantiation, ports and port accesses.

Port access rules are implemented as:

  • "local" port accesses (any port in the current or parent container hierarchy) can be accessed by a flat symbol name
  • instance port accesses can be accessed by a nested symbol access.
  • any instance in a parent container can be accessed.
  • Only the top-level ports of the instance can be accessed (arbitrary restriction).

Adds: container, container instantiation, ports and port accesses.

Port access rules are implemented as:
- "local" port accesses (any port in the current or parent container hierarchy) can be accessed by a flat symbol name
- instance port accesses can be accessed by a nested symbol access.
- any instance in a parent container can be accessed.
- Only the top-level ports of the instance can be accessed (arbitrary restriction).
Copy link
Contributor

@teqdruid teqdruid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is too general for my taste. Generality tends to introduce all sorts of corner cases. In my experience, it's better to be more restrictive at first then loosen those restrictions as necessary to support more use cases.

test/Dialect/Ibis/round-trip.mlir Outdated Show resolved Hide resolved
test/Dialect/Ibis/round-trip.mlir Outdated Show resolved Hide resolved
include/circt/Dialect/Ibis/IbisInterfaces.td Show resolved Hide resolved
@mortbopet
Copy link
Contributor Author

mortbopet commented Aug 2, 2023

This is too general for my taste. Generality tends to introduce all sorts of corner cases. In my experience, it's better to be more restrictive at first then loosen those restrictions as necessary to support more use cases.

I can get behind having top-level modules continue to be classes, instead of everything a container - it simplifies some of the symbol definition search stuff. Port access functions (as previously discussed) i think have plenty of merit in being general. I think a compromise here is: classes as top level, no nested containers (=> no container instantiation), port-related ops as-is.

EDIT: Dipping my toes into the lowering now, and I think I've changed my mind - instance port accesses are semantically different enough from local port accesses to warrant a separate op.

Copy link
Contributor

@teqdruid teqdruid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! all my comments are minor.

include/circt/Dialect/Ibis/IbisOps.td Show resolved Hide resolved
include/circt/Dialect/Ibis/IbisOps.td Outdated Show resolved Hide resolved
include/circt/Dialect/Ibis/IbisOps.td Outdated Show resolved Hide resolved
lib/Dialect/Ibis/IbisOps.cpp Outdated Show resolved Hide resolved
return op->emitOpError()
<< op->getName() << " must be contained in a ClassOp";

Operation *rootAccessOp = SymbolTable::lookupSymbolIn(parentClass, symName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a linear scan through the parentClass, right? If so, this is unacceptably inefficient longer term. Once we get the innerSym table stuff (on which I am pushing), it'll be more efficient. Please add a TODO comment reflecting this.

Also, doesn't the SymbolTable interface expose a method to do this?

Operation *rootAccessOp = SymbolTable::lookupSymbolIn(parentClass, symName);

TTargetOp targetOp;
if (auto getTargetRes = getTargetOp(rootAccessOp); succeeded(getTargetRes))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised you need to do the succeeded check separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FailureOr/LogicalResult is not and has never been implicitly convertible to bool.

lib/Dialect/Ibis/IbisOps.cpp Show resolved Hide resolved
lib/Dialect/Ibis/IbisOps.cpp Show resolved Hide resolved
lib/Dialect/Ibis/IbisOps.cpp Show resolved Hide resolved
ibis.port.input @C_in : i1
ibis.port.output @C_out : i1

ibis.instance @My_A, @A
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's stick to the camelCase for instance names and CamelCase for class name convention. (I'm a hypocrite here since I never stick to any conventions, but do as I say not as I do! ;) )

llvm::function_ref<Type(TSrcOp)> getPortType,
SymbolTableCollection &symbolTable) {
auto symName = op.getSymNameAttr();
auto localAccess = symName.getNestedReferences().empty();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beyond referencing ports in nested containers, I think we will need the ability to reference ports in parent/sibling containers. Something like a file system path like: ../../Foo/Bar/data. Will this code be able to be extended to support that?

include/circt/Dialect/Ibis/IbisOps.td Outdated Show resolved Hide resolved
@mortbopet mortbopet merged commit c3ed13d into main Aug 3, 2023
5 checks passed
@darthscsi darthscsi deleted the dev/mpetersen/ibis_lo branch June 4, 2024 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants