This is an archive of the discontinued LLVM Phabricator instance.

Differential D58589

[ConstantHoisting] Call cleanup() in ConstantHoistingPass::runImpl to avoid dangling elements in ConstIntInfoVec for new PM
ClosedPublic

Authored by MaskRay on Feb 23 2019, 11:03 PM.

Details

Reviewers
ormris
zzheng
dmgreen
wmi
Commits
rGf4b25f700a4c: [ConstantHoisting] Call cleanup() in ConstantHoistingPass::runImpl to avoid…
rL355174: [ConstantHoisting] Call cleanup() in ConstantHoistingPass::runImpl to avoid…
Summary

ConstIntInfoVec contains elements extracted from the previous function.
In new PM, releaseMemory() is not called and the dangling elements can
cause segfault in findConstantInsertionPoint.

Rename releaseMemory() to cleanup() to deliver the idea that it is
mandatory and call cleanup() in ConstantHoistingPass::runImpl to fix
this.

Diff Detail

Repository
rL LLVM

Event Timeline

MaskRay created this revision.Feb 23 2019, 11:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 23 2019, 11:03 PM
Herald added a subscriber: llvm-commits. · View Herald Transcript
Harbormaster completed remote builds in B28455: Diff 188071.Feb 23 2019, 11:04 PM
ormris added inline comments.Feb 25 2019, 10:43 AM
lib/Transforms/Scalar/ConstantHoisting.cpp
950 ↗(On Diff #188071)

If this only happens in the new pass manager, could this call be moved to ::run? That way, the legacy pass manager doesn't call this function twice.

ormris added inline comments.Feb 25 2019, 10:45 AM
lib/Transforms/Scalar/ConstantHoisting.cpp
950 ↗(On Diff #188071)

s/::run/ConstantHoistingPass::run/g

MaskRay updated this revision to Diff 188290.Feb 25 2019, 6:20 PM

Remove releaseMemory() and inline it into runImpl()

MaskRay marked 3 inline comments as done.Feb 25 2019, 6:21 PM
MaskRay added inline comments.
lib/Transforms/Scalar/ConstantHoisting.cpp
950 ↗(On Diff #188071)

Thanks! I think maybe it makes more sense to delete releaseMemory() and inline it into the end of runImpl. I've updated the patch to do that.

Harbormaster completed remote builds in B28525: Diff 188290.Feb 25 2019, 6:23 PM
MaskRay updated this revision to Diff 188293.Feb 25 2019, 6:31 PM
MaskRay marked an inline comment as done.
MaskRay retitled this revision from [ConstantHoisting] Call releaseMemory() to avoid dangling elements in ConstIntInfoVec to [ConstantHoisting] Inline releaseMemory() into ConstantHoistingPass::runImpl to avoid dangling elements in ConstIntInfoVec for new PM.
MaskRay edited the summary of this revision. (Show Details)

Update title

Harbormaster completed remote builds in B28526: Diff 188293.Feb 25 2019, 6:31 PM
ormris accepted this revision.Feb 26 2019, 9:21 AM

LGTM. I would wait for one other review before committing.

This revision is now accepted and ready to land.Feb 26 2019, 9:21 AM
wmi added inline comments.Feb 27 2019, 2:50 PM
lib/Transforms/Scalar/ConstantHoisting.cpp
950 ↗(On Diff #188071)

nit: why you feel it is better to inline it? It seems better to me to extract those cleanups into a function.

MaskRay marked an inline comment as done.Feb 27 2019, 5:27 PM
MaskRay added inline comments.
lib/Transforms/Scalar/ConstantHoisting.cpp
950 ↗(On Diff #188071)

releaseMemory() is called by legacy PM but not by new PM. This releaseMemory() doesn't do what people expected: this is not only related to release unused memory but also related to correctness. The data structure should be destroyed after applying on one function as otherwise it would become dangling.

MaskRay updated this revision to Diff 188845.Feb 28 2019, 9:10 PM
MaskRay retitled this revision from [ConstantHoisting] Inline releaseMemory() into ConstantHoistingPass::runImpl to avoid dangling elements in ConstIntInfoVec for new PM to [ConstantHoisting] Call cleanup() in ConstantHoistingPass::runImpl to avoid dangling elements in ConstIntInfoVec for new PM.
MaskRay edited the summary of this revision. (Show Details)

Rename releaseMemory() to cleanup() as wmi suggested.

Harbormaster completed remote builds in B28662: Diff 188845.Feb 28 2019, 9:11 PM
wmi accepted this revision.Feb 28 2019, 9:23 PM

LGTM

MaskRay added a comment.Feb 28 2019, 9:24 PM

Thank you for review!

Closed by commit rL355174: [ConstantHoisting] Call cleanup() in ConstantHoistingPass::runImpl to avoid… (authored by MaskRay). · Explain WhyFeb 28 2019, 9:27 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
llvm/
trunk/
include/
llvm/
Transforms/
Scalar/
2 lines
lib/
Transforms/
Scalar/
4 lines
test/
Transforms/
ConstantHoisting/
X86/
1 line

Diff 188846

llvm/trunk/include/llvm/Transforms/Scalar/ConstantHoisting.h

Show First 20 Lines • Show All 120 Lines • ▼ Show 20 Lines
class ConstantHoistingPass : public PassInfoMixin<ConstantHoistingPass> { class ConstantHoistingPass : public PassInfoMixin<ConstantHoistingPass> {
public: public:
PreservedAnalyses run(Function &F, FunctionAnalysisManager &AM); PreservedAnalyses run(Function &F, FunctionAnalysisManager &AM);
// Glue for old PM. // Glue for old PM.
bool runImpl(Function &F, TargetTransformInfo &TTI, DominatorTree &DT, bool runImpl(Function &F, TargetTransformInfo &TTI, DominatorTree &DT,
BlockFrequencyInfo *BFI, BasicBlock &Entry); BlockFrequencyInfo *BFI, BasicBlock &Entry);
void releaseMemory() { void cleanup() {
ClonedCastMap.clear(); ClonedCastMap.clear();
ConstIntCandVec.clear(); ConstIntCandVec.clear();
for (auto MapEntry : ConstGEPCandMap) for (auto MapEntry : ConstGEPCandMap)
MapEntry.second.clear(); MapEntry.second.clear();
ConstGEPCandMap.clear(); ConstGEPCandMap.clear();
ConstIntInfoVec.clear(); ConstIntInfoVec.clear();
for (auto MapEntry : ConstGEPInfoMap) for (auto MapEntry : ConstGEPInfoMap)
MapEntry.second.clear(); MapEntry.second.clear();
▲ Show 20 Lines • Show All 64 Lines • Show Last 20 Lines

llvm/trunk/lib/Transforms/Scalar/ConstantHoisting.cpp

Show First 20 Lines • Show All 108 Lines • ▼ Show 20 Linespublic:
void getAnalysisUsage(AnalysisUsage &AU) const override { void getAnalysisUsage(AnalysisUsage &AU) const override {
AU.setPreservesCFG(); AU.setPreservesCFG();
if (ConstHoistWithBlockFrequency) if (ConstHoistWithBlockFrequency)
AU.addRequired<BlockFrequencyInfoWrapperPass>(); AU.addRequired<BlockFrequencyInfoWrapperPass>();
AU.addRequired<DominatorTreeWrapperPass>(); AU.addRequired<DominatorTreeWrapperPass>();
AU.addRequired<TargetTransformInfoWrapperPass>(); AU.addRequired<TargetTransformInfoWrapperPass>();
} }
void releaseMemory() override { Impl.releaseMemory(); }
private: private:
ConstantHoistingPass Impl; ConstantHoistingPass Impl;
}; };
} // end anonymous namespace } // end anonymous namespace
char ConstantHoistingLegacyPass::ID = 0; char ConstantHoistingLegacyPass::ID = 0;
▲ Show 20 Lines • Show All 815 Lines • ▼ Show 20 Linesbool ConstantHoistingPass::runImpl(Function &Fn, TargetTransformInfo &TTI,
for (auto MapEntry : ConstGEPInfoMap) for (auto MapEntry : ConstGEPInfoMap)
if (!MapEntry.second.empty()) if (!MapEntry.second.empty())
MadeChange |= emitBaseConstants(MapEntry.first); MadeChange |= emitBaseConstants(MapEntry.first);
// Cleanup dead instructions. // Cleanup dead instructions.
deleteDeadCastInst(); deleteDeadCastInst();
cleanup();
return MadeChange; return MadeChange;
} }
PreservedAnalyses ConstantHoistingPass::run(Function &F, PreservedAnalyses ConstantHoistingPass::run(Function &F,
FunctionAnalysisManager &AM) { FunctionAnalysisManager &AM) {
auto &DT = AM.getResult<DominatorTreeAnalysis>(F); auto &DT = AM.getResult<DominatorTreeAnalysis>(F);
auto &TTI = AM.getResult<TargetIRAnalysis>(F); auto &TTI = AM.getResult<TargetIRAnalysis>(F);
auto BFI = ConstHoistWithBlockFrequency auto BFI = ConstHoistWithBlockFrequency
Show All 9 Lines

llvm/trunk/test/Transforms/ConstantHoisting/X86/phi.ll

; RUN: opt -S -consthoist < %s | FileCheck %s ; RUN: opt -S -consthoist < %s | FileCheck %s
; RUN: opt -S -passes=consthoist < %s | FileCheck %s
target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128" target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-apple-macosx10.9.0" target triple = "x86_64-apple-macosx10.9.0"
; PR18626 ; PR18626
define i8* @test1(i1 %cmp, i64* %tmp) { define i8* @test1(i1 %cmp, i64* %tmp) {
entry: entry:
call void @foo(i8* inttoptr (i64 68719476735 to i8*)) call void @foo(i8* inttoptr (i64 68719476735 to i8*))
▲ Show 20 Lines • Show All 107 Lines • Show Last 20 Lines