Page MenuHomePhabricator

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

Authored by InnovativeInventor on Wed, Oct 13, 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.Wed, Oct 13, 5:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Oct 13, 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
148–149
148–149

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

148–149

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.Wed, Oct 13, 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.Wed, Oct 13, 6:50 PM
InnovativeInventor added inline comments.
polly/lib/CodeGen/IslNodeBuilder.cpp
179

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
148–149

Please update the comment.

149–151

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.Thu, Oct 14, 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
179

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.Thu, Oct 14, 7:34 PM
InnovativeInventor edited the summary of this revision. (Show Details)EditedFri, Oct 15, 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.