P.S. we have not supported adding multiple, heterogeneous resource to an operation yet - current patch only makes sure that the introduction of resource does not break the current test cases.

To provide some additional context to other reviewers, the ability to have multiple resource types per operation is absolutely essential for practical HLS.

Thanks for the review, @andrewb1999 ! I have addressed the feedback you gave.

Moreover, we can now associate multiple resource types to a single operation now!

One more minor comment, but overall LGTM. I think it would be good if someone who has worked on the scheduling codebase more recently could give some feedback as well before merging.

I think it would be good if someone who has worked on the scheduling codebase more recently could give some feedback as well before merging.

@7FM maybe?

I'll have a look on Tuesday (not at my desk currently).

One more minor comment, but overall LGTM. I think it would be good if someone who has worked on the scheduling codebase more recently could give some feedback as well before merging.

Thank you for your review @andrewb1999 ! I will change it accordingly

I'll have a look on Tuesday (not at my desk currently).

If you could take a look, that'll be awesome @jopperm I'd really appreciate your input :)

I think it would be good if someone who has worked on the scheduling codebase more recently could give some feedback as well before merging.

@7FM maybe?

I just skimmed over the changes, LGTM so far. I am a huge fan of a clear separation between operator types and resources (especially being able to assign multiple resources to an operation)! This will certainly clean up my downstream resource sharing code base.

The only thing I was wondering about while skimming the change was the purpose of adding wrapper structs for StringAttr as Operator/ResourceType: https://github.com/llvm/circt/pull/8444/files#diff-0db8e1c62706227aae263d37b3263f7c010ca77640a1329f3980bf83e1379f3aR98-R140

Right now, this does not seem to add functionality, but adds some boilerplate. Could you give a rationale for adding them? Is it to provide a common interface for Resource- and OperatorTypes to simplify changing the underlying classes for downstream users? (I don't think I have tried/needed this yet, but maybe you could give an example use case if that was indeed the intent)

I just skimmed over the changes, LGTM so far. I am a huge fan of a clear separation between operator types and resources (especially being able to assign multiple resources to an operation)! This will certainly clean up my downstream resource sharing code base.

The only thing I was wondering about while skimming the change was the purpose of adding wrapper structs for StringAttr as Operator/ResourceType: https://github.com/llvm/circt/pull/8444/files#diff-0db8e1c62706227aae263d37b3263f7c010ca77640a1329f3980bf83e1379f3aR98-R140

Right now, this does not seem to add functionality, but adds some boilerplate. Could you give a rationale for adding them? Is it to provide a common interface for Resource- and OperatorTypes to simplify changing the underlying classes for downstream users? (I don't think I have tried/needed this yet, but maybe you could give an example use case if that was indeed the intent)

Re.:

Right now, this does not seem to add functionality, but adds some boilerplate.

You are right, right now, my focus is to make sure the smooth transition from using OperatorType soley to separating to OperatorType and ResourceType, so I did was to just to make sure the separation design makes sense and it should not break the current CI. We will add more interesting features later.

Re.:

Could you give a rationale for adding them?

Yes, there is a rationale:

1. It enables type-level distinction between conceptually different things

• Even though both types are backed by a StringAttr, operator types and resource types represent different scheduling concepts, as we all agree.
• Without wrappers, we'd risk using a StringAttr intended for one concept in place of another.

2. And it provides a foundation for future extensions

• We will have more interesting scheduling cases and ResourceType will then actually be different from OperatorType, probably by adding different utility functions to different types, attach richer metadata etc.

Please let me know if you have any question. Thanks for your review and feedback, it's really helpful :)

Thanks for working on this -- I agree this will be super useful and looks very neat already.

Have you considered tying the resources to the operator types instead of the operations?

Sorry for the late reply, I got seriously blocked by something over the week.

I did not consider until you ask, and I think this is a really good question. Here is what I think after some pondering, and I will answer the question below together since I think you are trying to probe the same thing?

Have you considered using just one shared container for operator types and resources?

My understanding is I feel like separating them both conception-wise and implementation-wise/container-wise is more neat, because operator types represents what an operation does and resource types tells us where or how the operation is being run.

Since there will be the same operator using different resource types, or the same resource type is shared across operators, combing the operator type and resource type container altogether will lead to duplicate resource declaration in different containers.

And correct me if I'm wrong, if we tie resource types to the operator type (by which I understand your proposal as instead of putting operation in the first hierarchy and both resource/operator type on the second hierarchy, tying resource types to operator types means that resource type will be on the third hierarchy?), then resource types cannot be shared across problems.

Thanks again for your review and valuable insight @jopperm and please let me know if there's any further concern :)

Thanks for clarifying!

Since there will be the same operator using different resource types [...]

I think that's the point I didn't consider initially. Then it makes sense to set operator type and resources separately per operation. What's your canonical example for this? Something like "memory read port" as the operator type, and "memory_XYZ" as the resource?

I think that's the point I didn't consider initially. Then it makes sense to set operator type and resources separately per operation. What's your canonical example for this? Something like "memory read port" as the operator type, and "memory_XYZ" as the resource?

Yeah something like that, I was thinking to have memory_read as the operator type since "read" is a verb, just like an operator to perform; and memory_read_port as the resource since "ports" are resources.

Can you please update the documentation in a follow-up PR as well, please?

Sure, will do!

Merging the PR, thanks folks again for taking your time to review! 🙏