This is an archive of the discontinued LLVM Phabricator instance.

Differential D122633

[mlir][OpenMP] Fix memory leak by deleting unused value
ClosedPublic

Authored by shraiysh on Mar 28 2022, 10:09 PM.

Details

Reviewers
ftynse
jdoerfert
rriddle
Commits
rG4d1010909f2e: [mlir][OpenMP] Fix memory leak by deleting unused value

Diff Detail

Event Timeline

shraiysh created this revision.Mar 28 2022, 10:09 PM
Herald added a reviewer: ftynse. · View Herald TranscriptMar 28 2022, 10:09 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: awarzynski, sdasgup3, wenzhicui and 22 others. · View Herald Transcript
shraiysh requested review of this revision.Mar 28 2022, 10:09 PM
Herald added a reviewer: jdoerfert. · View Herald TranscriptMar 28 2022, 10:09 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: sstefan1, stephenneuendorffer, nicolasvasilache. · View Herald Transcript
shraiysh mentioned this in D122483: [flang][OpenMP] Added allocate clause translation for OpenMP block constructs.Mar 28 2022, 10:10 PM
shraiysh mentioned this in D121554: [mlir][OpenMP] Added translation from `omp.atomic.capture` to LLVM IR.
shraiysh added a reviewer: rriddle.
shraiysh updated this revision to Diff 418792.Mar 28 2022, 10:51 PM

Using eraseFromParent instead of deleteValue.

ftynse accepted this revision.Mar 29 2022, 1:26 AM
This revision is now accepted and ready to land.Mar 29 2022, 1:26 AM
rriddle accepted this revision.Mar 29 2022, 7:59 AM
rriddle added inline comments.
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
1222

Can you also sink this variable into the if? It isn't used anywhere else.

This revision was landed with ongoing or failed builds.Mar 29 2022, 2:56 PM
Closed by commit rG4d1010909f2e: [mlir][OpenMP] Fix memory leak by deleting unused value (authored by shraiysh). · Explain Why
This revision was automatically updated to reflect the committed changes.
shraiysh added a commit: rG4d1010909f2e: [mlir][OpenMP] Fix memory leak by deleting unused value.
Harbormaster completed remote builds in B156697: Diff 418792.Mar 29 2022, 10:13 PM

Revision Contents

PathSize
mlir/
lib/
Target/
LLVMIR/
Dialect/
OpenMP/
1 line

Diff 418785

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

Show First 20 Lines • Show All 1,213 Lines • ▼ Show 20 Lines auto updateFn = [&](llvm::Value *atomicx,
omp::YieldOp yieldop = dyn_cast<omp::YieldOp>(bb.getTerminator()); omp::YieldOp yieldop = dyn_cast<omp::YieldOp>(bb.getTerminator());
assert(yieldop && yieldop.results().size() == 1 && assert(yieldop && yieldop.results().size() == 1 &&
"terminator must be omp.yield op and it must have exactly one " "terminator must be omp.yield op and it must have exactly one "
"argument"); "argument");
return moduleTranslation.lookupValue(yieldop.results()[0]); return moduleTranslation.lookupValue(yieldop.results()[0]);
}; };
// Handle ambiguous alloca, if any. // Handle ambiguous alloca, if any.
auto allocaIP = findAllocaInsertPoint(builder, moduleTranslation); auto allocaIP = findAllocaInsertPoint(builder, moduleTranslation);
llvm::UnreachableInst *unreachableInst; llvm::UnreachableInst *unreachableInst;
rriddleUnsubmitted
Not Done
ReplyInline Actions

Can you also sink this variable into the if? It isn't used anywhere else.

rriddle: Can you also sink this variable into the if? It isn't used anywhere else.
if (allocaIP.getPoint() == ompLoc.IP.getPoint()) { if (allocaIP.getPoint() == ompLoc.IP.getPoint()) {
// Same point => split basic block and make them unambigous. // Same point => split basic block and make them unambigous.
unreachableInst = builder.CreateUnreachable(); unreachableInst = builder.CreateUnreachable();
builder.SetInsertPoint(builder.GetInsertBlock()->splitBasicBlock( builder.SetInsertPoint(builder.GetInsertBlock()->splitBasicBlock(
unreachableInst, "alloca_split")); unreachableInst, "alloca_split"));
ompLoc.IP = builder.saveIP(); ompLoc.IP = builder.saveIP();
unreachableInst->removeFromParent(); unreachableInst->removeFromParent();
unreachableInst->deleteValue();
} }
builder.restoreIP(ompBuilder->createAtomicCapture( builder.restoreIP(ompBuilder->createAtomicCapture(
ompLoc, findAllocaInsertPoint(builder, moduleTranslation), llvmAtomicX, ompLoc, findAllocaInsertPoint(builder, moduleTranslation), llvmAtomicX,
llvmAtomicV, llvmExpr, atomicOrdering, binop, updateFn, atomicUpdateOp, llvmAtomicV, llvmExpr, atomicOrdering, binop, updateFn, atomicUpdateOp,
isPostfixUpdate, isXBinopExpr)); isPostfixUpdate, isXBinopExpr));
return updateGenStatus; return updateGenStatus;
} }
▲ Show 20 Lines • Show All 172 Lines • Show Last 20 Lines