This is an archive of the discontinued LLVM Phabricator instance.

[NFC] [Polly] Convert GPUNodeBuilder::getArraySize to islcpp.
ClosedPublic

Authored by bollu on Jul 23 2017, 3:29 AM.

Event Timeline

bollu created this revision.Jul 23 2017, 3:29 AM
grosser edited edge metadata.Jul 23 2017, 1:27 PM

Except of the minor issue, that looks fine to me.

lib/CodeGen/PPCGCodeGeneration.cpp
1021

Use isl::manage(isl_multi_pw_aff_copy(Array->bound));

Otherwise, you take ownership of th the reference in Array->bound and you will free it unexpectedly in the future.

1033

Drop this line. It is not needed after the change above. What you did here is to extract the object out of ArrayBound, but then ignore the return value and let it leak. This just works, because it matches up with the error above,

grosser accepted this revision.Jul 23 2017, 1:27 PM
This revision is now accepted and ready to land.Jul 23 2017, 1:27 PM
bollu added inline comments.Jul 24 2017, 1:26 AM
lib/CodeGen/PPCGCodeGeneration.cpp
1033

Yes, I did it this way to create a "temporary" C++ interface that I then release. However, I suppose copying and then allowing the destructor to manage memory is less error prone.

I don't think the concept of a "temporary C++ interface" exists. To me it looks like too memory errors that hide each other. We we at some point write a static analyzer to check for memory errors, it will believe that the Array->bound object is invalid after your original code sequence.

This revision was automatically updated to reflect the committed changes.

The commit r308869 was incorrectly labeled - it was supposed to close D35770. Writing this here as documentation.

bollu reopened this revision.Jul 24 2017, 1:44 AM
This revision is now accepted and ready to land.Jul 24 2017, 1:44 AM
bollu added a comment.Jul 24 2017, 1:49 AM

@grosser I do not believe so, release sets the ptr within the isl::multi_pw_aff to nullptr. So, the destructor will not free anything.

isl-multi-pw-aff-release.cpp
__isl_give isl_multi_pw_aff *multi_pw_aff::release() {
  isl_multi_pw_aff *tmp = ptr;
  ptr = nullptr;
  return tmp;
}

It shouldn't leak memory, it just looks awkward.

bollu updated this revision to Diff 107865.Jul 24 2017, 1:52 AM
  • [NFC wrt patch] copy Array->bound, don't acquire and release
bollu added a comment.Jul 24 2017, 2:01 AM
This comment was removed by bollu.
This revision was automatically updated to reflect the committed changes.