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

[FXML-4791] Add printout of references with emitc.reference attr #316

Merged
merged 20 commits into from
Sep 9, 2024

Conversation

cferry-AMD
Copy link
Collaborator

@cferry-AMD cferry-AMD commented Aug 28, 2024

So, here's an attempt to have some form of references with EmitC. This PR merely consists of:

  • an attribute emitc.reference that, when borne by function arguments or cast operations, results in a reference variable being created,
  • some printout code for these references.

To make things smoother for the arrival of a reference type (if that's what upstream would like to see), the printout of types is split into two functions: printReferenceToType, and printType (the existing function). For now, printReferenceToType calls printType but not the other way around, so only the outermost type can bear references (there's no way to make an array of refs for instance); a true reference type would allow nesting and therefore back-and-forth calls between printReferenceToType and printType.

@cferry-AMD cferry-AMD force-pushed the corentin.references branch from fc11685 to 61d5346 Compare August 28, 2024 15:45
@cferry-AMD cferry-AMD marked this pull request as ready for review August 29, 2024 07:17
@cferry-AMD cferry-AMD changed the title Add printout of references with emitc.reference attr [FXML-4791] Add printout of references with emitc.reference attr Aug 29, 2024
@cferry-AMD
Copy link
Collaborator Author

One thing that seems to be common is to define named attributes of ops in the ODS. Should we also do this for the reference attribute?

@cferry-AMD
Copy link
Collaborator Author

I think we'll have to live with the {emitc.reference} attribute of arguments, as the function op parser doesn't give any flexibility (we can't have a dialect-specific function argument parser). Additionally, it seems highly unwise to make a change impacting all other dialects - Shape, Func, etc - to accomodate a special use of EmitC, especially if a true reference type can work without these changes...

Copy link
Collaborator

@mgehre-amd mgehre-amd left a comment

Choose a reason for hiding this comment

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

Looks good!

@cferry-AMD
Copy link
Collaborator Author

cferry-AMD commented Sep 6, 2024

I ended up duplicating most of the code in mlir/lib/Interfaces/FunctionImplementation.cpp into mlir/lib/Dialect/EmitC/IR/FunctionOpAssembly.cpp. Now we get a nice printout of function signatures:

emitc.func @f(%x: i32 ref)

@cferry-AMD
Copy link
Collaborator Author

Something tells me upstream won't like the duplicated code very much. But they might be fine with amending the function interfaces that will set custom parsers for arguments and likewise results (in which case we wouldn't have to duplicate so much code, only the code that does the argument list parsing).

mlir/test/Target/Cpp/global.mlir Show resolved Hide resolved
@cferry-AMD cferry-AMD merged commit dec1017 into feature/fused-ops Sep 9, 2024
4 checks passed
@cferry-AMD cferry-AMD deleted the corentin.references branch September 9, 2024 07:01
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.

3 participants