[Polly] Generating code for changed memory accesses

[Tobias Grosser]

On 24/07/2014 22:52, Johannes Doerfert wrote:
> Hey Tobias, Hey Yabin,
>
>
>
> I integrated the comments you gave me on the dev-list and put a patch
> together which serves my needs for now.
>
> The only real missing piece I see are multidimensional accesses we need to
> support once we want to switch delinearization on.

That's fine with me.

> [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.

> +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.

> +  // 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]

After addressing these comments, this patch is good to go.

Thanks Johannes, this was quick and super useful!

Cheers,
Tobias