This is an archive of the discontinued LLVM Phabricator instance.

[Polly] Move computations out of constructors
ClosedPublic

Authored by Meinersbur on Jul 24 2015, 12:44 PM.

Details

Summary

It is common practice to keep constructors lightweight. The reasons include:

  • The vtable during the constructor's execution is set to the static type of the object, not to the vtable of the derived class. That is, method calls behave differently in constructors and ordinary methods. This way it is possible to call unimplemented methods of abstract classes, which usually results in a segmentation fault.
  • If an exception is thrown in the constructor, the destructor is not called, potentially leaking memory.
  • Code in constructors cannot be called in a regular way, e.g. from non-constructor methods of derived classes.
  • Because it is common practice, people (including me) do not expect the constructor to do more than initializing data and skip them when looking for bugs.

Not all of these are applicable to LLVM (e.g. exceptions are disabled).

This patch refactors out the computational work in the constructors of Scop, ScopStmt and IslAst into regular init functions and introduces static create-functions as replacement. The constructor ScopArrayInfo also has calls, but it is small and does not call methods of its own class.

Diff Detail

Event Timeline

Meinersbur retitled this revision from to [Polly] Move computations out of constructors.
Meinersbur updated this object.
Meinersbur added a reviewer: grosser.
Meinersbur added a project: Restricted Project.
Meinersbur added subscribers: pollydev, llvm-commits.
grosser accepted this revision.Jul 26 2015, 3:11 PM
grosser edited edge metadata.

Hi Michael,

I think this looks good to me. Just a minor thing. Could you possibly make the constructors
and init() functions private such that only the create*() functions are part of the public interface
and any user is always sure to get a fully initialized class.

Best,
Tobias

This revision is now accepted and ready to land.Jul 26 2015, 3:11 PM

I did where I could (IslAst)

ScopStmt and MemoryAccess ctors are used in emplace_back/emplace_font. I can change these only if I change the std::vector to store pointers to the objects (or dirty tricks like creating at a different place and the move it into the vector).

Actually, changing these vectors may be a good idea - so the objects do not need to be movable anymore. What do you think?

Meinersbur added a comment.EditedJul 27 2015, 5:29 AM

Note: they are not stored in std::vector, but std::deque and std::forward_list, otherwise we'd have a problem if these objects are being relocated.

I'm not sure about the whole "moving code out of constructors" part [1] but I am sure about two things:

  1. It is not worth it to introduce things one would call "dirty tricks".
  2. Introducing explicit memory managment or pointers to objects that are allocated at different places is always error prone. Our forward_lists and vectors with emplaced objects might not be the best solution but at least easy to reason about wrt. memory.

[1] I never experience any of the claimed problems hence I do not see the need for a change. However, if the new code/interface is at least as clean as the current one I do not see a problem either.

Hi Michael,

just a vector of ScopStmt pointers is what we had before and we moved away of this for memory safety reasons (see discussion '[polly] r238090 - Use unique_ptr to clarify ownership of ScopStmt' on llvm-commits). I agree with Johannes, that we should not add new hacks to move out computations from the constructor and also don't think that having both public factor methods and public (partial) constructors is better than what we have now.However, I also have currently a good suggestion of how to do it better.

The islAst part seems non-controversial. Feel free to commit it if you like. Also, maybe add some comments into the header.

This revision was automatically updated to reflect the committed changes.