cbalint13 commented on PR #16425: URL: https://github.com/apache/tvm/pull/16425#issuecomment-2022171896
> > > Here is a reproducer: > > > mem_leak.cpp > > > > > > Thanks a lot for this, I start to look at it now. > > @lhutton1 , > > Here is a patch: [tvm-llvm-memleak.diff.gz](https://github.com/apache/tvm/files/14762596/tvm-llvm-memleak.diff.gz) Can confirm that on your side is fine ? > > Later EDIT: the patch is against vanilla tvm latest. @lhutton1 , Few comments on the fix here: * First these LLVM module interrogation functions creates it's **own sessions** independent from the main instance. * It seems that within same context (the LLVMTargetInfo class) there will be multiple LLVM instances going memleak I wanted these functions as separate class apart from ```TVMTargetInfo``` but was a requirement at review time of the PR 1. Your fix **is not naive** at all: https://github.com/lhutton1/tvm/commit/31927904b378c142be3044419e38501425805cd4 - You force creation within a smart pointers until the needed SubtargetInfo interface, this is a effective way IMO 2. [My fix](https://github.com/apache/tvm/files/14762596/tvm-llvm-memleak.diff.gz) only **make sure** to re-use the llvm_instance + llvm_target_machine using existing ```GetOrCreate*``` interface. - But we **loose now the constness** of the querying functions here - All query parameters (tripe_, cpu_, attrs_) **now must be passed** at ```LLVMTargetInfo``` class creation --- I honestly would go with your #1 , because it **keeps the constness** and **the independent parameters** of querying. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
