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

Import FX graph node name #131

Draft
wants to merge 1 commit into
base: feature/backport_ea1_ops
Choose a base branch
from

Conversation

mgehre-amd
Copy link
Collaborator

No description provided.

@mgehre-amd mgehre-amd force-pushed the matthias.import_node_debugName branch from 57ed408 to d7d21fa Compare July 28, 2023 11:10
@mgehre-amd mgehre-amd force-pushed the matthias.import_node_debugName branch from d7d21fa to 983ce1a Compare August 9, 2023 09:31
@mgehre-amd mgehre-amd changed the title Matthias.import node debug name Import FX graph node name Aug 9, 2023
@mgehre-amd mgehre-amd marked this pull request as ready for review August 9, 2023 09:31
@mgehre-amd mgehre-amd force-pushed the matthias.import_node_debugName branch from 983ce1a to f719bf6 Compare August 10, 2023 06:37
@mgehre-amd mgehre-amd force-pushed the matthias.import_node_debugName branch from f719bf6 to caca87b Compare August 10, 2023 09:14
@mgehre-amd
Copy link
Collaborator Author

@cmcgirr-amd, I cleaned this up and added test. I think we can go ahead and merge this.

@cmcgirr-amd
Copy link

@mgehre-amd did you add the test here or in another repo?

@mgehre-amd
Copy link
Collaborator Author

@mgehre-amd did you add the test here or in another repo?

Oh shoot, I was thinking about another PR. I will add a test here.


// Whether to add the FXOutputName attribute from the debug name of the jit
// node.
bool addFxOutputName = false;
Copy link

@cmcgirr-amd cmcgirr-amd Aug 11, 2023

Choose a reason for hiding this comment

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

In the context of torch-mlir this output name would be the tensor "SSA" name we see in the FX graph right? Maybe its worth mentioning it in the comment.

We have just been filling OutputName with the name of the operation that created the output. Might be confusing.

@mgehre-amd mgehre-amd marked this pull request as draft June 21, 2024 13:48
@cmcgirr-amd cmcgirr-amd requested a review from ttjost August 2, 2024 09:34
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