This is an archive of the discontinued LLVM Phabricator instance.

[Polly][NFC] Remove checkIslAstExprInt and use RAII instead of manually freeing Expr
ClosedPublic

Authored by InnovativeInventor on Oct 13 2021, 5:18 PM.

Details

Summary

Polly is trying to move towards using isl::ast_expr/ isl-noexceptions.h (which implements RAII) where possible instead of manually managing memory. checkIslAstExprInt manually frees Expr, so it has been removed to be more idiomatic and consistent.

Diff Detail

Event Timeline

InnovativeInventor requested review of this revision.Oct 13 2021, 5:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 13 2021, 5:18 PM

Could you update the second parameter of checkIslAstExprInt as well? Yes, it's cheating in that it passes isl functions as callbacks which you will not be able to do anymore, but it also prohibits us to use proper use of RAII.

This time you actually need NFC to the title.

polly/lib/CodeGen/IslNodeBuilder.cpp
151–152

Use Expr.isa<isl::ast_expr_int>()

151–154
153–154

Expr.val()

InnovativeInventor retitled this revision from [Polly] Switch checkIslAstExprInt to use RAII instead of manually freeing Expr to [Polly][NFC] Switch checkIslAstExprInt to use RAII instead of manually freeing Expr.Oct 13 2021, 6:43 PM

Would be better to make this a function inside the ast_expr class? I could also overload the == operator for either ist::val or ist::ast_expr or both (not sure if this is a good idea though).

InnovativeInventor marked 3 inline comments as done.Oct 13 2021, 6:50 PM
InnovativeInventor added inline comments.
polly/lib/CodeGen/IslNodeBuilder.cpp
187

It looks like I should change this to be !Expr.isa<isl::ast_expr_int>() as well, right? If so, should I make a different review or do you want each patch to be tightly scoped and not contain a hodgepodge of changes?

Would be better to make this a function inside the ast_expr class?

No. The ast_expr class is automatically generated and your changes will be lost when re-generating.

I could also overload the == operator for either ist::val or ist::ast_expr or both

While is possible to overload operator== independently of isl::ast_expr/isl::val, this would be something that the C++ wrapper would typically do. We have Polly-specific overloads for operator<<, but you would need to make a case that this would be useful elsewhere to make such a global change.

polly/lib/CodeGen/IslNodeBuilder.cpp
149–150

Please update the comment.

152–154

Its becoming simple enough that you could just inline the function.

Using isl::val::is_zero/isl::val::is_one would be slightly more efficient.

InnovativeInventor retitled this revision from [Polly][NFC] Switch checkIslAstExprInt to use RAII instead of manually freeing Expr to [Polly][NFC] Remove checkIslAstExprInt and use RAII instead of manually freeing Expr.

I ended up removing checkIslAstExprInt since it can be inlined.

Fair point -- thanks for being so patient!

Meinersbur accepted this revision.Oct 14 2021, 7:34 PM

LGTM.

Could you update the summary to something that makes sense in a commit message? Yes, you understand correctly.

Would you like me to land this patch for you?

polly/lib/CodeGen/IslNodeBuilder.cpp
187

I assume this was a comment to an earlier revision and you meant UB.isa<... as it is in the current iteration.

Looks god as-is.

This revision is now accepted and ready to land.Oct 14 2021, 7:34 PM
InnovativeInventor edited the summary of this revision. (Show Details)EditedOct 15 2021, 1:21 AM
InnovativeInventor edited the summary of this revision. (Show Details)

LGTM.

Could you update the summary to something that makes sense in a commit message? Yes, you understand correctly.

Would you like me to land this patch for you?

Yes, thanks! I just revised the summary to make more sense.