This is an archive of the discontinued LLVM Phabricator instance.

[Polly][Ast] Partial refactoring of IslAst and IslAstInfo to use isl++. NFC.
ClosedPublic

Authored by patacca on Apr 11 2021, 7:22 AM.

Details

Summary

Polly use algorithms from the Integer Set Library (isl), which is a library written in C and which is incompatible with the rest of the LLVM as it is written in C++.

Changes made:

  • Refactoring the following methods of class IslAst
    • getAst() getRunCondition() buildRunCondition()
    • Removed the destructor in favor of the default one
  • Change the type of the attribute IslAst.RunCondition to isl::ast_expr
  • Change the type of the attribute IslAst.Root to isl::ast_node
  • Change the order of attributes in class IslAst to reflect the data dependencies so that the destructor won't complain
  • Refactoring the following methods of class IslAstInfo
    • getAst() getRunCondition()

Diff Detail

Event Timeline

patacca created this revision.Apr 11 2021, 7:22 AM
patacca published this revision for review.Apr 11 2021, 9:57 AM
patacca added a project: Restricted Project.
Herald added a project: Restricted Project. · View Herald TranscriptApr 11 2021, 9:57 AM
patacca added a comment.EditedApr 11 2021, 10:02 AM

There is the question on how we want to access the isl::ast_expr object RunCondition.
Right now I declared RunCondition as type isl::ast_expr so the object lives inside IslAst class. Unfortunately the C++ bindings do not define yet the move constructor nor the move assignment so when doing

RunCondition(std::move(O.RunCondition))
Condition = IslAst::buildRunCondition(*S, Build)

we are relying on compiler optimizations to eliminate the unnecessary copies.
I think there is the idea of implementing move constructor and move assignment in the future for isl bindings so it might be ok to just leave the code like this in the mean time.
Otherwise with a little trick we can do r-value assignment like this:

Condition = isl::manage(O.RunCondition.release())

It feels a bit ugly though.

Another way would be to use unique_ptr<isl::ast_expr> instead of the object itself. That way we do not need isl defined move constructor and move assigment.

patacca retitled this revision from [Polly][NFC] Refactoring IslAst and partially IslAstInfo to use isl++ to [Polly][NFC][DO NOT LAND YET] Refactoring IslAst and partially IslAstInfo to use isl++.Apr 11 2021, 10:05 AM
patacca updated this revision to Diff 337160.Apr 13 2021, 8:29 AM

Changelog:

  • Completing the refactoring of IslAst::buildRunCondition()
Meinersbur added a comment.EditedApr 13 2021, 11:25 PM

It is correct that the addition of move semantics was intended to be done eventually. Thus would be one of the things to do for the GSoC project.

Until then we accepted that without these move semantics, there are redundant calls to *_copy and *_free calls. More critically, it may lead to additional object copies when calling a __isl_take function, but the reference count is not 1 because of the redundant reference. However, these do not seem to be that significant for the overall performance, so while it could be better, we know that it cane be improved with better bindings. For polly::IslAst::RunCondition, there is always another reference to it (by polly::IslAst::RunCondition) so any __isl_take function will always need to make a copy and the only overhead are the additional *_copy and *_free calls.

That is, don't worry about making additional copies for now.

Btw, other functionality is missing from the isl-noexceptions.h bindings as well, such as most callbacks.

patacca updated this revision to Diff 337826.EditedApr 15 2021, 10:49 AM
patacca retitled this revision from [Polly][NFC][DO NOT LAND YET] Refactoring IslAst and partially IslAstInfo to use isl++ to [Polly][NFC] Partial refactoring of IslAst and IslAstInfo to use isl++.
patacca edited the summary of this revision. (Show Details)

Changelog:

  • Remove destructor for class IslAst in favor of default one
  • Convert IslAst.Root to isl++
  • Change the order of attributes in class IslAst to reflect the data dependencies so that the destructor won't complain

If it looks good to a reviewer I think we can now land it

Meinersbur accepted this revision.Apr 15 2021, 12:40 PM

Change the order of attributes in class IslAst to reflect the data dependencies so that the destructor won't complain

Nice observation. Did it crash before changing the order?

Could you move the NFC designation to the end of the title? IMHO [NFC] makes the impression that there is component called "NFC". The component here would be IslAst, which you could add as well.
Such as

[Polly][Ast] Partial refactoring of IslAst and IslAstInfo to use isl++. NFC.

I know that there is not consistent placement of NFC in LLVM commits, just stating my personal preference.

The LLVM coding standard uses auto only in very specific cases. In the past we made liberal use of auto, but when changing a declaration we can use the occasion to bring it to the latest coding standard.No need to update this patch because of this since it is unrelated with what the patch is doing, but something to consider for future patches.

Would you like me to push the patch to main?

polly/include/polly/CodeGen/IslAst.h
40

Removing custom dtors is always nice.

polly/lib/CodeGen/IslAst.cpp
409

The isl-noexceptions.h don't implement move ctors/assignments. so std::move currently has no effect.

Still nice to consider move-semantics for when it finally is supported.

This revision is now accepted and ready to land.Apr 15 2021, 12:40 PM

Change the order of attributes in class IslAst to reflect the data dependencies so that the destructor won't complain

Nice observation. Did it crash before changing the order?

Yes, it failed some tests. If I remember correctly it was ending up inside isl_ctx_free() with a reference counter > 0 and thus calling isl_die().

The LLVM coding standard uses auto only in very specific cases. In the past we made liberal use of auto, but when changing a declaration we can use the occasion to bring it to the latest coding standard.No need to update this patch because of this since it is unrelated with what the patch is doing, but something to consider for future patches.

I didn't think too much about it, I will keep it in mind in the future.

Please, go ahead and push it to main.

patacca retitled this revision from [Polly][NFC] Partial refactoring of IslAst and IslAstInfo to use isl++ to [Polly][Ast] Partial refactoring of IslAst and IslAstInfo to use isl++. NFC..Apr 15 2021, 3:22 PM
This revision was landed with ongoing or failed builds.Apr 15 2021, 10:40 PM
This revision was automatically updated to reflect the committed changes.