Skip to content

Conversation

imaihal
Copy link
Collaborator

@imaihal imaihal commented Nov 5, 2024

This PR releases MLIRContext and Module before execution opt command. They are not necessary after writing LLVM bitcode. By releasing the module, the peak memory reduced a bit.

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>
Co-authored-by: Yasushi Negishi <negishi@jp.ibm.com>
// Free memory before using LLVM `opt` command
llvmModule.reset();
module.release();
context.~MLIRContext();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you know what are freed/released with these statements? It looks not too much memory saved and not sure it is worth to make a change.

// Return 0 on success, error code on failure
static int compileModuleToObject(const mlir::OwningOpRef<ModuleOp> &module,
std::string outputNameWithoutExt, std::string &objectNameWithExt) {
static int compileModuleToObject(mlir::OwningOpRef<ModuleOp> &module,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Removing const and freeing the object internally seems difficult to follow, at least by looking at the function name. Same for other signature changes in other functions.

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>
std::string sharedLibNameWithExt;
int rc = compileModuleToSharedLibrary(
module, outputNameNoExt, sharedLibNameWithExt);
module, outputNameNoExt, sharedLibNameWithExt, context);
Copy link
Collaborator

@tungld tungld Nov 8, 2024

Choose a reason for hiding this comment

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

@imaihal just be noticed that module and context are potentially used later by outputCode (See Line 813/826). So please make sure outputCode still works since module has been released inside compileModuleToSharedLibrary.

@imaihal imaihal marked this pull request as draft November 26, 2024 04:41
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