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

[binder] Implement OM #3626

Merged
merged 1 commit into from
Dec 1, 2023

Conversation

SpriteOvO
Copy link
Member

  • Add OM C-API.
  • Extract PassManager to a abstract class.
  • Implement property IR builds.

This PR is still working in progress, we will update more details once it is ready for review.

Copy link
Member

@sequencer sequencer left a comment

Choose a reason for hiding this comment

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

Map and tuple is removed.

@SpriteOvO
Copy link
Member Author

Map and tuple is removed.

Rebased to the main branch for changes in #3627. Map and tuple supports are removed now.

binder/src/main/scala/PanamaCIRCT.scala Outdated Show resolved Hide resolved
Comment on lines 114 to 126
val obj = evaluator.instantiate("PropertyTest_Class", Seq(om.newBasePathEmpty))
val path = obj.field("p").asInstanceOf[PanamaCIRCTOMEvaluatorValuePath].asString

assert(path == "OMReferenceTarget:~PropertyTest|PropertyTest>i")

converter.mlirStream
}) should include("om.class.field @p")
.and(include("om.class.field @a, %3 : !om.list<!om.list<!om.integer>>"))
.and(include("om.class.field @b, %3 : !om.list<!om.list<!om.integer>>"))
}
Copy link
Member

Choose a reason for hiding this comment

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

Good first try

@SpriteOvO
Copy link
Member Author

This PR depends on llvm/circt#6413 for the property types API.

@SpriteOvO SpriteOvO force-pushed the binder-om branch 3 times, most recently from 81c8cda to 5d7e59a Compare November 13, 2023 05:51
Copy link
Member

@sequencer sequencer left a comment

Choose a reason for hiding this comment

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

From a design perspective, this PR lgtm, wait for circt next version to get it in.

@sequencer
Copy link
Member

rebase it plz.

@SpriteOvO SpriteOvO changed the base branch from main to ci/ci-circt-nightly November 30, 2023 11:34
@SpriteOvO SpriteOvO force-pushed the binder-om branch 3 times, most recently from 05ea3f6 to 1e6e043 Compare November 30, 2023 11:38
@SpriteOvO SpriteOvO changed the title Implement OM C-API for Binder [binder] Implement OM Nov 30, 2023
@SpriteOvO SpriteOvO force-pushed the binder-om branch 3 times, most recently from bd81f82 to df76b08 Compare December 1, 2023 09:43
@SpriteOvO SpriteOvO marked this pull request as ready for review December 1, 2023 10:10
Copy link
Member

@sequencer sequencer left a comment

Choose a reason for hiding this comment

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

Due to #3627, Corresponding C-API should be removed in CIRCT, please CIRCT to remove them to reduce the C-API size.

Comment on lines +158 to +159
omTypeIsAMapType
omMapTypeGetKeyType
Copy link
Member

Choose a reason for hiding this comment

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

Map and Tuple are removed in #3627, please make sure.

Comment on lines +186 to +189
omEvaluatorMapGetElement
omEvaluatorMapGetKeys
omEvaluatorValueIsAMap
omEvaluatorMapGetType
Copy link
Member

Choose a reason for hiding this comment

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

Map and Tuple are removed in #3627, please make sure.

Comment on lines +205 to +209
# MapAttr API
omAttrIsAMapAttr
omMapAttrGetNumElements
omMapAttrGetElementKey
omMapAttrGetElementValue
Copy link
Member

Choose a reason for hiding this comment

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

Map and Tuple are removed in #3627, please make sure.

Comment on lines +183 to +185
omEvaluatorValueIsATuple
omEvaluatorTupleGetNumElements
omEvaluatorTupleGetElement
Copy link
Member

Choose a reason for hiding this comment

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

Map and Tuple are removed in #3627, please make sure.

@SpriteOvO
Copy link
Member Author

Due to #3627, Corresponding C-API should be removed in CIRCT, please CIRCT to remove them to reduce the C-API size.

They are not the same thing, explained here #3626 (comment).

@sequencer
Copy link
Member

I'd like to merge this commit, but @SpriteOvO need to do some clean up in CIRCT. We are maintaining a lot of C-APIs in the binder, we need to clean up the unused APIs

@sequencer
Copy link
Member

They are not the same thing, explained here #3626 (comment).

Interesting.

@sequencer sequencer merged commit 7a0c77f into chipsalliance:ci/ci-circt-nightly Dec 1, 2023
9 checks passed
@SpriteOvO SpriteOvO deleted the binder-om branch December 1, 2023 10:15
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.

None yet

2 participants