[PATCH] [Polly] Introduce the ScopArrayInfo class.

[Tobias Grosser]

Very nice. Besides fixing two important bugs in the run-time alias checks, this is also conceptually a big step forward. Thanks for working on this last night!

The patch looks great, with just some test-case changes that I unfortunately do not understand.

I  also added a couple (3-4) of minor comments on your code. None of them has correctness implications and the original code is already very nice, so just include them as you like.

================
Comment at: include/polly/CodeGen/IslExprBuilder.h:84
@@ -83,2 +83,3 @@
   ///
+  /// @param S       The current SCoP.
   /// @param Builder The IRBuilder used to construct the isl_ast_expr[ession].
----------------
It is unclear to me why the IslExpr Builder would need a SCoP now.

================
Comment at: lib/Analysis/ScopInfo.cpp:294
@@ +293,3 @@
+const ScopArrayInfo *
+ScopArrayInfo::getFromAccessFunction(__isl_keep isl_pw_multi_aff *PMA) {
+  isl_id *Id = isl_pw_multi_aff_get_tuple_id(PMA, isl_dim_out);
----------------
A similar function that takes an isl_id and returns the ScopArrayInfo might be handy too. (See later comment on how to obtain the BasePtr/ElementType of an isl_ast_op_access).

================
Comment at: lib/Analysis/ScopInfo.cpp:475
@@ -444,1 +474,3 @@
+  void *User = const_cast<ScopArrayInfo *>(SAI);
+  isl_id *BaseAddrId = isl_id_alloc(Ctx, getBaseName().c_str(), User);
 
----------------
Conceptually it probably makes sense to allocate one isl_id in the ScopArrayInfo class and then give users the option to obtain copies of this id. This makes clear that all these isl_ids are identical. (The current code also works at the moment AFAIK, as isl assumes two ids are identical if their name and the user pointer are identical).

================
Comment at: lib/CodeGen/IslCodeGeneration.cpp:58
@@ -58,5 +57,3 @@
 public:
-  IslNodeBuilder(PollyIRBuilder &Builder, ScopAnnotator &Annotator, Pass *P,
-                 LoopInfo &LI, ScalarEvolution &SE, DominatorTree &DT)
-      : Builder(Builder), Annotator(Annotator), ExprBuilder(Builder, IDToValue),
-        P(P), LI(LI), SE(SE), DT(DT) {}
+  IslNodeBuilder(Scop &S, PollyIRBuilder &Builder, ScopAnnotator &Annotator,
+                 Pass *P, LoopInfo &LI, ScalarEvolution &SE, DominatorTree &DT)
----------------
I am surprised we need the Scop here.

================
Comment at: lib/CodeGen/IslExprBuilder.cpp:113
@@ +112,3 @@
+
+  const ScopArrayInfo *SAI = S.getScopArrayInfo(Base);
+  if (Base->getType() != SAI->getType())
----------------
Instead of passing the scop to this place and use the base ptr to look up the ScopArrayInfo, you could possibly also obtain the isl_id from the isl_ast_expr_get_op_arg(Expr, 0) and then use a "static ScopArrayInfo::getFromid(Id)" function.

Also, instead of obtaining the Base pointer by calling create(), we could obtain
it directly from this ScopArrayInfo object. This would allow us to remove 
IslNodeBuilder::addMemoryAccesses(), which was mostly added due to us missing a ScopArrayInfo class.

================
Comment at: lib/CodeGen/IslExprBuilder.cpp:137
@@ -136,3 +121,1 @@
-         "generation.");
 
-  Access = Builder.CreateGEP(Base, Indices, "polly.access." + Base->getName());
----------------
Nice cleanup!

================
Comment at: test/Isl/CodeGen/MemAccess/codegen_simple.ll:43
@@ -42,2 +42,2 @@
 }
-; CHECK: load i32* getelementptr inbounds ([100 x i32]* @A, i64 0, i64 0)
+; CHECK: load i32* getelementptr inbounds ([100 x i32]* @A, i{{(32|64)}} 0, i{{(32|64)}} 0)
----------------
It is unclear to me why this test case needs to be modified and especially why the generated IR now can be have one of two forms. Is the IR we generate not supposed to be predictable?

http://reviews.llvm.org/D5613