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

[InnovativeInventor]

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.

[Meinersbur]

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

[style] don't use braces for single statements

148–149

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

148–149

Expr.val()

[InnovativeInventor]

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]

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?

[Meinersbur]

In D111769#3063023, @InnovativeInventor wrote:

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]

I ended up removing checkIslAstExprInt since it can be inlined.

[InnovativeInventor]

Fair point -- thanks for being so patient!

[Meinersbur]

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.

[InnovativeInventor]

In D111769#3065913, @Meinersbur wrote:

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.

polly/lib/CodeGen/IslNodeBuilder.cpp

isl::ast_expr IslNodeBuilder::getUpperBound(isl::ast_node_for For,
 
   isl::id IteratorID = Iterator.get_id();
 
   assert(UBID.get() == IteratorID.get() &&
          "conditional expression is not an atomic upper bound");
 
   return Cond.get_op_arg(1);
 }
 
-/// Return true if a return value of Predicate is true for the value represented
-/// by passed isl_ast_expr_int.
-static bool checkIslAstExprInt(__isl_take isl_ast_expr *Expr,
-                               isl_bool (*Predicate)(__isl_keep isl_val *)) {
-  if (isl_ast_expr_get_type(Expr) != isl_ast_expr_int) {
-    isl_ast_expr_free(Expr);
-    return false;
-  }
-  auto ExprVal = isl_ast_expr_get_val(Expr);
-  isl_ast_expr_free(Expr);
-  if (Predicate(ExprVal) != isl_bool_true) {
-    isl_val_free(ExprVal);
-    return false;
-  }
-  isl_val_free(ExprVal);
-  return true;
-}
-
 int IslNodeBuilder::getNumberOfIterations(isl::ast_node_for For) {
   assert(isl_ast_node_get_type(For.get()) == isl_ast_node_for);
   isl::ast_node Body = For.body();
 
   // First, check if we can actually handle this code.
   switch (isl_ast_node_get_type(Body.get())) {
   case isl_ast_node_user:
     break;
   case isl_ast_node_block: {
     isl::ast_node_block BodyBlock = Body.as<isl::ast_node_block>();
     isl::ast_node_list List = BodyBlock.children();
     for (isl::ast_node Node : List) {
       isl_ast_node_type NodeType = isl_ast_node_get_type(Node.get());
       if (NodeType != isl_ast_node_user)
         return -1;
     }
     break;
   }
   default:
     return -1;
   }
 
   isl::ast_expr Init = For.init();
-  if (!checkIslAstExprInt(Init.release(), isl_val_is_zero))
+  if (!Init.isa<isl::ast_expr_int>() || !Init.val().is_zero())
     return -1;
   isl::ast_expr Inc = For.inc();
-  if (!checkIslAstExprInt(Inc.release(), isl_val_is_one))
+  if (!Inc.isa<isl::ast_expr_int>() || !Inc.val().is_one())
     return -1;
   CmpInst::Predicate Predicate;
   isl::ast_expr UB = getUpperBound(For, Predicate);
-  if (isl_ast_expr_get_type(UB.get()) != isl_ast_expr_int)
+  if (!UB.isa<isl::ast_expr_int>())
     return -1;
   isl::val UpVal = UB.get_val();
   int NumberIterations = UpVal.get_num_si();
   if (NumberIterations < 0)
     return -1;
   if (Predicate == CmpInst::ICMP_SLT)
     return NumberIterations;
   else