[PATCH] D35471: [Polly] [RFC] Calculate AST expression type

[Tobias Grosser via Phabricator via llvm-commits]

grosser added a comment.

Hi Maximilian,

this looks indeed very good! Thanks for pushing this forward! I did a first round with stylistic comments.

Could you also please add test cases for some of the code generation changes. I will do another round with comments on the content of the patch itself soonish.



================
Comment at: include/polly/CodeGen/IslExprBuilder.h:197
+  /// Inject \p Bound into the expression generator. Normally, a node in the
+  /// AST tree is preceded by it's bound, therefore it's sufficient to store
+  /// the bound in @link CurrentBound before descending.
----------------
preceeded


================
Comment at: include/polly/CodeGen/IslExprBuilder.h:203
+  /// manually inject a bound for the next expression before calling
+  /// @link create
+  ///
----------------
Add "." at end of sentence?


================
Comment at: include/polly/CodeGen/IslExprBuilder.h:260
+  /// node. To use them, we store the bound in @link CurrentBound before
+  /// generating then inner expression which is then read by @link getType when
+  /// asked for the expression's type.
----------------
THE inner


================
Comment at: lib/CodeGen/IslAst.cpp:525
+      Cond = extractPreconditionFromAST(isl_ast_expr_get_op_arg(Expr, i), Cond);
+    }
+    isl_ast_expr_free(Expr);
----------------
No braces around single statement bodies.


================
Comment at: lib/CodeGen/IslExprBuilder.cpp:735
+      llvm_unreachable("Unbound expression!");
+    } else {
+      int size = isl_ast_expr_get_bound_bits(CurrentBound);
----------------
No need for else, If the previous branch condition holds, we anyhow assert.


================
Comment at: lib/CodeGen/IslExprBuilder.cpp:742
+      // more useful to only use this exact for annotations and only use this
+      // for code generation on specific request
+      if (size < 64)
----------------
"." at end of sentence. Also, can you check the grammar. Something seems wrong.


================
Comment at: lib/CodeGen/IslNodeBuilder.cpp:96
+  // TODO: XXX: Why is this here? This shouldn't do anything, is this a bug?
   isl_ast_expr_get_type(Cond);
   assert(isl_ast_expr_get_type(Cond) == isl_ast_expr_op &&
----------------
I removed this in r309533. Thank you for spotting!


================
Comment at: lib/CodeGen/IslNodeBuilder.cpp:553
+  // we can simply use getType(Iterator), but in the compute case, we have to
+  // do this differently...
+  if (isl_ast_expr_get_type(Iterator) == isl_ast_expr_bound_t) {
----------------
What is the "compute case"?


================
Comment at: lib/External/isl/isl_ast_build_expr.c:2738
+    if (res->type == isl_ast_expr_op)
+        res = isl_ast_bin_op_set_size(res, data->build);
 	return res;
----------------
Indentation?


================
Comment at: lib/External/isl/isl_ast_build_private.h:208
+
+    	struct isl_hash_table *cache;
 };
----------------
Indentation?


================
Comment at: lib/External/isl/isl_ast_codegen.c:1442
 	depth = isl_ast_build_get_depth(build);
+
 	id = isl_ast_build_get_iterator_id(build, depth);
----------------
Unrelated change.


https://reviews.llvm.org/D35471