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

[ESI] Add AppID design hierarchy manifest data to the manifest #6345

Merged
merged 23 commits into from
Oct 26, 2023

Conversation

teqdruid
Copy link
Contributor

@teqdruid teqdruid commented Oct 25, 2023

Serialize the IR representation into JSON as part of the manifest.

@teqdruid teqdruid added the ESI label Oct 25, 2023
@teqdruid teqdruid marked this pull request as ready for review October 25, 2023 23:08
@teqdruid
Copy link
Contributor Author

zlib is really good at compressing JSON:

image

It was so small, I actually verified it with Python's zlib library!

@teqdruid teqdruid merged commit 58662f4 into main Oct 26, 2023
5 checks passed
@teqdruid teqdruid deleted the dev/teqdruid/esi/appid-manifest branch October 26, 2023 20:45
Copy link
Contributor

@mortbopet mortbopet left a comment

Choose a reason for hiding this comment

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

post-merge review.

@@ -112,6 +113,10 @@ def ServiceImplRecordOp : ESI_Op<"esi.manifest.service_impl", [
qualified($appID) (`svc` $service^)? `by` $serviceImplName `with` $implDetails
attr-dict-with-keyword $reqDetails
}];

let extraClassDeclaration = [{
void getDetails(SmallVectorImpl<NamedAttribute> &results);
Copy link
Contributor

Choose a reason for hiding this comment

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

'getDetails' is pretty vague to me - referencing the summary, does this mean that the details are the "information necessary to connect to the service and service clients"? - it probably should have a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method is part of the IsMetadata OpInterface and is described there... in not too much more detail.

@@ -89,6 +89,7 @@ def ServiceRequestRecordOp : ESI_Op<"esi.manifest.req", [
AppIDAttr getAppID() {
return getRequestor();
}
void getDetails(SmallVectorImpl<NamedAttribute> &results);
Copy link
Contributor

Choose a reason for hiding this comment

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

as above.

@@ -131,6 +136,10 @@ def ServiceImplClientRecordOp : ESI_Op<"esi.manifest.impl_conn", [
$relAppIDPath `req` $servicePort `(` $bundleType `)`
`with` $implDetails attr-dict
}];

let extraClassDeclaration = [{
void getDetails(SmallVectorImpl<NamedAttribute> &results);
Copy link
Contributor

Choose a reason for hiding this comment

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

as above.

// Manifest ops.
//===----------------------------------------------------------------------===//

StringRef ServiceImplRecordOp::getManifestClass() { return "service"; }
Copy link
Contributor

Choose a reason for hiding this comment

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

can these raw strings be moved to some named constants?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They're not (as of now) used in any other part of the code -- they simply appear in the manifest. So I don't think that warrants a named constant.

// Gather the relevant types.
mod->walk([&](Operation *op) { scrapeTypes(op); });
// Find the top level appid hierarchy root.
for (auto root : mod->getRegion(0).front().getOps<AppIDHierRootOp>())
Copy link
Contributor

Choose a reason for hiding this comment

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

Operation *mod = getOperation(); -> mlir::ModuleOp mod = getOperation(); since you anyways restricting pass scheduling at that level.
Then you don't need this getRegion(0).front() but can just refer to the body block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm trying to write code that is resilient to future changes in the outer container (the coming hw.design outer container).

@teqdruid
Copy link
Contributor Author

6ef2489

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants