This is an archive of the discontinued LLVM Phabricator instance.

[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

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
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.

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

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.

wmi accepted this revision.Feb 28 2019, 9:23 PM

LGTM

Thank you for review!

This revision was automatically updated to reflect the committed changes.