Skip to content

Fix LLVM debug info #57

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

Merged
merged 3 commits into from
Aug 18, 2025
Merged

Fix LLVM debug info #57

merged 3 commits into from
Aug 18, 2025

Conversation

frabert
Copy link
Collaborator

@frabert frabert commented Jul 28, 2025

No description provided.

@frabert frabert marked this pull request as ready for review July 30, 2025 09:29
@frabert frabert requested a review from Jezurko July 30, 2025 09:31
Copy link
Collaborator

@Jezurko Jezurko left a comment

Choose a reason for hiding this comment

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

It's quite similar to what I had in mind :), but I think we are reinventing some parts MLIR already provides.

Comment on lines 83 to 88
struct ModuleScope {
std::optional<SymbolTable::UseRange> symbolUses;

ModuleScope(ModuleOp module) : symbolUses(*SymbolTable::getSymbolUses(module)) {}
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would try to replace this with the SymbolUserMap which does something very similar, but provides some utility capabilities.


std::shared_ptr<IRMapping> mapping;
OpBuilder builder;
SymbolTable symbolTable;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would probably have to be replaced with SymbolTableCollection (because for some reason the SymbolUserMap requires that)… Maybe shared in a similar way as the IRMapping, since it holds multiple SymbolTables?
It can be used to build (and then hold) the symbol tables on demand

ModuleScope(ModuleOp module) : symbolUses(*SymbolTable::getSymbolUses(module)) {}
};

static LogicalResult renameRemappedUsersOf(Operation *op, const ModuleScope &scope, StringAttr newName,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not 100 % sure, but maybe most of this can be handled by replaceAllUsesWith from the SymbolUserMap

@frabert frabert force-pushed the frabert/fix-debug-info branch from 91f53d8 to edb21cd Compare August 14, 2025 09:29
@frabert
Copy link
Collaborator Author

frabert commented Aug 14, 2025

@Jezurko I tried using SymbolTableCollection and SymbolUserMap in the last commit but it broke a bunch of tests :/

@Jezurko
Copy link
Collaborator

Jezurko commented Aug 14, 2025

Hm, that's a pity :/ I will give it a quick look what's happening

@frabert frabert force-pushed the frabert/fix-debug-info branch from f1bd67b to 46ca027 Compare August 18, 2025 09:02
@frabert frabert merged commit 66ee00d into main Aug 18, 2025
14 checks passed
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