This is an archive of the discontinued LLVM Phabricator instance.

Fixed a memory leak in the PDLToPDLInterp RootOrderingTest.
AbandonedPublic

Authored by sfuniak on Nov 29 2021, 4:46 AM.

Details

Summary

RootOrderingTest is a low-level unit test that creates values and uses them as vertices in a directed graph. These vertices were created using builder.create, but never freed, due to my insufficient understanding of the MLIR infrastructure.

Diff Detail

Event Timeline

sfuniak created this revision.Nov 29 2021, 4:46 AM
sfuniak requested review of this revision.Nov 29 2021, 4:46 AM
bondhugula requested changes to this revision.Nov 29 2021, 4:51 AM
bondhugula added inline comments.
mlir/unittests/Conversion/PDLToPDLInterp/RootOrderingTest.cpp
38–39

You need to be erasing those ops using erase().

39

As you know here that getDefiningOp() is a ConstantOp, please use a way to assert. For eg:

assert(v[i].getDefiningOp());
cast<ConstantOp>(v[i].getDefiningOp()).erase();
This revision now requires changes to proceed.Nov 29 2021, 4:51 AM
bondhugula added inline comments.Nov 29 2021, 4:52 AM
mlir/unittests/Conversion/PDLToPDLInterp/RootOrderingTest.cpp
38

Nit: unsigned here and below.

46–47

Please use ConstantIntOp -- that way, you can just pass i as the arg.

sfuniak updated this revision to Diff 390477.Nov 29 2021, 2:46 PM

Review feedback.

Herald added a reviewer: sstefan1. · View Herald Transcript
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a reviewer: MaskRay. · View Herald Transcript
Herald added a reviewer: baziotis. · View Herald Transcript
Herald added a reviewer: sjarus. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
sfuniak abandoned this revision.Nov 29 2021, 2:48 PM

Messed up a rebase, will submit a new diff.

mehdi_amini added inline comments.Nov 29 2021, 4:00 PM
mlir/unittests/Conversion/PDLToPDLInterp/RootOrderingTest.cpp
38

@bondhugula unsigned is an anti-pattern (unless working with bit-fields or other bit-manipulation), this has been widely documented I believe, both in C++ standardization paper (they are stuck with size_t in the standard library) and with conference talks like this one: https://www.youtube.com/watch?v=yG1OZ69H_-o