[FIRRTL] DomainFieldAttr types are PropertyTypes #9094
Conversation
seldridge
force-pushed
the
dev/seldridge/firrtl-verify-domain-field-type-is-property-ods
branch
from
f2ad599 to
3454bc5
Compare
dtzSiFive
left a comment
dtzSiFive
left a comment
There was a problem hiding this comment.
LGTM.
I think the reorg is right, or at least nothing seems wrong about it to me and the changes make sense (and I think are a bit tidier than things were).
| #include "circt/Dialect/FIRRTL/FIRRTLDialect.h.inc" | ||
|
|
||
| // Pull in all enum type definitions and utility function declarations. | ||
| #include "circt/Dialect/FIRRTL/FIRRTLEnums.h.inc" |
There was a problem hiding this comment.
Was including this a problem?
Looking at upstream MLIR dialect headers, they seem to.. do things differently.
I think requiring including FIRRTLEnums.h separately is reasonable to match how we've organized attributes/operations/types similarly.
Good call, thanks!
There was a problem hiding this comment.
It was a problem if this was included by FIRRTLEnums.h (which the PR needed) as the .h.inc don't have any include guards. Some have the GET_<kind>_CLASSES guard which could help, but enums and attributes are totally unguarded. Hence, I needed to get this under something that we could include guard.
I was following along with how SPIRV dialect was defined upstream and modeling it off of that.
Yes, we are doing things differently from upstream and that makes me want to go redo our dialects to align with upstream...
|
And of course the change to PropertyType's is great and LGTM! Thanks for the cleanup! |
seldridge
force-pushed
the
dev/seldridge/firrtl-verify-domain-field-type-is-property-ods
branch
from
3454bc5 to
9045bf9
Compare
Split the FIRRTL ODS description of enums from attributes. This was all combined and creates problems if more upstream ODS files that describe attributes (FIRRTLAttributes.td) need to include from more downstream ODS files (FIRRTLTypes.ts). Signed-off-by: Schuyler Eldridge <[email protected]>
Add verification (via ODS) that the types of DomainFieldAttrs are PropertyTypes as opposed to generic MLIR types. Signed-off-by: Schuyler Eldridge <[email protected]>
seldridge
force-pushed
the
dev/seldridge/firrtl-verify-domain-field-type-is-property-ods
branch
from
9045bf9 to
f47a6fe
Compare
seldridge
deleted the
dev/seldridge/firrtl-verify-domain-field-type-is-property-ods
branch
Split the FIRRTL ODS description of enums from attributes. This was all
combined and creates problems if more upstream ODS files that describe
attributes (FIRRTLAttributes.td) need to include from more downstream ODS
files (FIRRTLTypes.ts).
Add verification (via ODS) that the types of DomainFieldAttrs are
PropertyTypes as opposed to generic MLIR types.