[llvm] [SPIR-V]: Memory leak fix in SPIRVEmitIntrinsics (PR #83015)
via llvm-commits
llvm-commits at lists.llvm.org
Tue Feb 27 08:57:51 PST 2024
================
@@ -660,33 +669,34 @@ bool SPIRVEmitIntrinsics::runOnFunction(Function &Func) {
AggrStores.insert(&I);
}
- IRB->SetInsertPoint(&Func.getEntryBlock(), Func.getEntryBlock().begin());
+ B.SetInsertPoint(&Func.getEntryBlock(), Func.getEntryBlock().begin());
for (auto &GV : Func.getParent()->globals())
- processGlobalValue(GV);
+ processGlobalValue(GV, B);
- preprocessUndefs();
- preprocessCompositeConstants();
+ preprocessUndefs(B);
+ preprocessCompositeConstants(B);
SmallVector<Instruction *> Worklist;
for (auto &I : instructions(Func))
Worklist.push_back(&I);
for (auto &I : Worklist) {
- insertAssignPtrTypeIntrs(I);
- insertAssignTypeIntrs(I);
- insertPtrCastInstr(I);
+ insertAssignPtrTypeIntrs(I, B);
+ insertAssignTypeIntrs(I, B);
+ insertPtrCastInstr(I, B);
}
-
+ this->IRB = &B;
----------------
bwlodarcz wrote:
Passing B everywhere would be the best option but unfortunately we can't use `B` as the reference in whole function because visit* methods have strict interface and changing them fails to compile definition files.
The problem with `this->IRB` approach is that lifecycle of the resource is not visible from the interface of the functions.
The class logic flow looks like that:
1. enter `runOnFunction`
2. assign pointer to IRB
3. call private helper methods
4. run visit
5. exit `runOnFunction`
The flow isn't visible in class and raises questions instantaneously when approached:
1. When IRB is created?
2. Can I use private methods before I run `runOnFunction`? - no
3. When IRB is again a `nullptr`?
4. How the class is used? Isn't IRB pointer passed somewhere to deeper frame or returned to higher?
The uncertain lifecycle of IRB was IMO the actual cause of the error.
By changing interfaces of the private methods to require the reference sends visible signal to everybody reading the code that IRBuilder needs to exist to call private method.
I think that clearer fix would be to add support class which will take visit* interfaces on itself. That PR would be likely bigger than this one.
https://github.com/llvm/llvm-project/pull/83015
More information about the llvm-commits
mailing list