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

Introduce OM to T1 project #495

Merged
merged 2 commits into from
May 7, 2024
Merged

Introduce OM to T1 project #495

merged 2 commits into from
May 7, 2024

Conversation

SpriteOvO
Copy link
Member

@SpriteOvO SpriteOvO commented Apr 1, 2024

@sequencer sequencer force-pushed the binder-om branch 3 times, most recently from 6169a1e to 7a529e3 Compare April 18, 2024 22:09
@SpriteOvO SpriteOvO force-pushed the binder-om branch 4 times, most recently from 93be2ca to 249cc30 Compare April 30, 2024 17:53
omreaderlib/src/Interface.scala Outdated Show resolved Hide resolved
omreaderlib/src/Interface.scala Outdated Show resolved Hide resolved
omreaderlib/src/Interface.scala Show resolved Hide resolved
})
}

def dumpAll(): Unit = {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def dumpAll(): Unit = {
def dumpOM(): Unit = {

Copy link
Member

Choose a reason for hiding this comment

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

what's the purpose of this function?

Copy link
Member Author

Choose a reason for hiding this comment

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

Dump the whole tree of the input instantiated OM object.

Comment on lines 18 to 22
def run(
@arg(name = "mlirbc-file") mlirbcFile: Option[os.Path],
@arg(name = "class-name") className: String,
@arg(name = "dump-methods") dumpMethods: Flag,
@arg(name = "eval") eval: Option[String],
) = {
Copy link
Member

Choose a reason for hiding this comment

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

I prefer API to be as simple as possible, this API is used by doc/PnR/DV guys. We should statically maintain them.

  def dumpOM(
    @arg(name = "mlirbc-file") mlirbcFile: os.Path,
    @arg(name = "class-name") className: String,
  ): String = ???
  def vlen(
    @arg(name = "mlirbc-file") mlirbcFile: os.Path,
    @arg(name = "class-name") className: String,
  ): String = ???
  def dlen(
    @arg(name = "mlirbc-file") mlirbcFile: os.Path,
    @arg(name = "class-name") className: String,
  ): String = ???
  1. mlirbcFile: just use mlirbcFile, we generally don't use read from stdin.
  2. className should be automatically detected, users don't need know what Class is, or force it to be fixed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense. I have pushed a commit for it, could you review it again please?

omreader/src/Main.scala Show resolved Hide resolved
@SpriteOvO SpriteOvO force-pushed the binder-om branch 2 times, most recently from fa36604 to 3d87d85 Compare May 6, 2024 11:25
println(simplyGetT1Reader(mlirbcFile).dlen)
}

def simplyGetT1Reader(mlirbcFile: os.Path) = OMReader.fromFile(mlirbcFile, "T1Subsystem_Class").t1Reader
Copy link
Member

Choose a reason for hiding this comment

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

We need to support both T1_Class and T1Subsystem_Class

@SpriteOvO SpriteOvO force-pushed the binder-om branch 2 times, most recently from 9f09ce2 to 4ae371d Compare May 6, 2024 13:29
nix/overlay.nix Outdated Show resolved Hide resolved
nix/t1/om.nix Outdated Show resolved Hide resolved
@SpriteOvO SpriteOvO force-pushed the binder-om branch 2 times, most recently from 638f5ca to ee38539 Compare May 7, 2024 14:41
sequencer
sequencer previously approved these changes May 7, 2024
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.

wait for ci

@sequencer
Copy link
Member

This bump doesn't include the 1.74 version of circt...

@SpriteOvO SpriteOvO merged commit 126b7f4 into master May 7, 2024
82 checks passed
@SpriteOvO SpriteOvO deleted the binder-om branch May 7, 2024 21:21
@sequencer
Copy link
Member

Cc @SharzyL this api is used to handle all metadatas our c header and emu should use it.

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.

2 participants