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

[Tobias Grosser via Phabricator via llvm-commits]

grosser added a comment.

Hi Maximilian,

thank you for pushing your WIP-patch. I know it still must be cleaned up, but here some comments which would make it easier to reason about the current patch:

1. Many test cases fail because we print casts when printing the generated AST. Can you disable the printing of these casts? They only distract and removing them makes it a lot easier to understand what is still failing,

2. In ScopInfo/int2ptr_ptr2int.ll and some others the IR is changed. Do you have an idea why? I would expect for us to not yet change the IR, no?

Best,
Tobias



================
Comment at: include/polly/CodeGen/IslAst.h:86
+                                           isl_ast_expr *cond,
+                                           isl_ast_build *Build);
 };
----------------
Could you add documentation?


================
Comment at: include/polly/CodeGen/IslExprBuilder.h:188
+  void injectBound(isl_ast_expr *Bound);
+
 private:
----------------
Could you add documentation?


================
Comment at: include/polly/CodeGen/IslExprBuilder.h:226
   llvm::Value *createInt(__isl_take isl_ast_expr *Expr);
+  llvm::Value *createBound(__isl_take isl_ast_expr *Expr);
   llvm::Value *createOpAddressOf(__isl_take isl_ast_expr *Expr);
----------------
Could you add documentation?

(I know there is not too much documentation around, but as this function is doing something rather special it would be good to document it).


================
Comment at: include/polly/CodeGen/IslExprBuilder.h:272
+protected:
+  isl_ast_expr *CurrentBound = nullptr;
 };
----------------
Could you add documentation?


================
Comment at: lib/CodeGen/IslAst.cpp:433
+    isl_options_set_ast_build_compute_bounds(Ctx, true);
+  }
   isl_ast_build *Build;
----------------
Just write:

```
  isl_options_set_ast_build_compute_bounds(Ctx, ComputeBounds);
```

without the if-condition around.


================
Comment at: lib/CodeGen/IslAst.cpp:434
+  }
   isl_ast_build *Build;
   AstBuildUserInfo BuildInfo;
----------------
Could you add flags to be able to control all configurations?


================
Comment at: lib/CodeGen/IslAst.cpp:515
+  return cond;
+}
+
----------------
Would it make sense to use ```isl_ast_node_foreach_descendant_top_down```?


================
Comment at: lib/CodeGen/IslExprBuilder.cpp:20
 #include "llvm/Transforms/Utils/BasicBlockUtils.h"
+#include <iostream>
 
----------------
No need to include iostream, just use llvm::errs()


================
Comment at: lib/CodeGen/IslExprBuilder.cpp:731
+      isl_printer_free(p);
+      std::cerr << std::endl << "END EXPR" << std::endl;
+      llvm_unreachable("Unbound expression!");
----------------
``llvm::errs() <<`` is what we use by default.


================
Comment at: lib/CodeGen/IslNodeBuilder.cpp:532
+  // When codegenerating this tree, the visitor would store the bound before
+  // desccending, but we're not using the visitor. In the non-compute case,
+  // we can simply use getType(Iterator), but in the compute case, we have to
----------------
descending


================
Comment at: lib/External/isl/isl_ast_graft_private.h:102
 
+
 #endif
----------------
Unrelated


================
Comment at: lib/External/isl/isl_options_private.h:62
+	int			ast_build_compute_bounds;
+   	int			ast_build_approximate_computed_bounds;
+	int 		ast_build_maximum_native_type;
----------------
Alignment?


https://reviews.llvm.org/D35471