[Polly] Generating code for changed memory accesses

[Johannes Doerfert]

Hey,

The last version of the patch still has some flaws. One of which is caused by tiling/pre-vectorization.
Once the optimizer changes the number of dimensions, the access relation does not match the schedule anymore.
Fixing this is "easy" as we only need to apply the scattering of the schedule before we eliminate constant dimensions.
However, we still have a problem if the "AST-codegen" will eliminate dimensions 
And this iteration is not fixed in our access relation after applying the schedule (e.g. when we prevectorize a loop with 3 iterations).

We somehow need to perform the same "transformation" as the AST-codegen to get an access relation which matches our actual schedule.
Any thoughts on how to do that?

Best regards,
  Johannes

--

Johannes Doerfert
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation


-----Original Message-----
From: Johannes Doerfert [mailto:doerfert at cs.uni-saarland.de] 
Sent: Monday, July 28, 2014 1:18 AM
To: Tobias Grosser
Cc: Johannes Doerfert; 'Yabin Hu'; llvm-commits at cs.uiuc.edu
Subject: Re: [Polly] Generating code for changed memory accesses

On 07/25, Tobias Grosser wrote:
> >[This also breaks the little functionality the jscop-cloog interface 
> >offered, but I don't think that should stop us at this point.]
> 
> I am OK with breaking CLooG as long as we move the test cases such 
> that we both preserve test coverage and do not cause failing test cases.
I moved & updated them to work with the isl codegen backend.

> >+Value *IslExprBuilder::createOpAccess(__isl_take isl_ast_expr *Expr) 
> >+{
> >+  assert(isl_ast_expr_get_type(Expr) == isl_ast_expr_op &&
> >+         "isl ast expression not of type isl_ast_op");
> >+  assert(isl_ast_expr_get_op_type(Expr) == isl_ast_op_access &&
> >+         "not an access isl ast expression");
> >+  assert(isl_ast_expr_get_op_n_arg(Expr) >= 2 &&
> >+         "We need at least two operands in an n-ary operation");
> >+
> >+  Value *Base = create(isl_ast_expr_get_op_arg(Expr, 0));
> >+
> >+  SmallVector<Value *, 4> IndexArray;  for (int i = 1; i < 
> >+ isl_ast_expr_get_op_n_arg(Expr); ++i) {
> >+    Value *OpV = create(isl_ast_expr_get_op_arg(Expr, i));
> >+    OpV = Builder.CreateSExt(OpV, getType(Expr));
> >+    IndexArray.push_back(OpV);
> >+  }
> 
> I would prefer to just assert in case we have more than one array 
> dimensions and to remove this loop. As you state above, we most likely 
> don't want to push those values into IndexArray, but instead compute 
> the linearized access ourselves. Instead of committing code that is 
> currently unused and likely to change later, I prefer to state the 
> limitation and only commit the code later.
I have to go back on what I said earlier, we need the IndexArray (we already have a unit test for that case). The test basically looks like this:

int A[1024]

void f()
  for i
     A[i]

Now the gep needs two indices [0, i].
I catch this case by checking the base address type, but I'm not yet a hundred percent sure that this is the right/best solution.

> >+  // TODO: In case of multiple dimensions we need to compute the 
> >+ offset ourselfs
> ourselves
> 
> >+  //       since the Base is likely a simple pointer. To do so we need access to
> >+  //       the memory access associated with the "original base" as it stores
> >+  //       the delinearization information.
> 
> I do not understand the second sentence.
> 
> [I would just leave out the todo or shorten it to 'Support for 
> multi-dimensional arrays]
Done.

> After addressing these comments, this patch is good to go.
I made one additional change, I turned this kind of access generation on by default. Is that OK with you or not? (The idea is to simplify/remove the BlockGenerator step by step).

Best regards,
  Johannes