[Polly] Generating code for changed memory accesses

[Yabin Hu]

Hi Johannes,

Very nice patches!
These cover almost everything I did in my GSoC project. I don't mean to
interrupt the wonderful discussion between Tobias and you (actually I
learned a lot from your discussions), so I just talk about a little
difference when I implemented BlockGenerator class.

BlockGenerator::BlockGenerator(PollyIRBuilder &B, ScopStmt &Stmt, Pass *P,
IslExprBuilder *ExprBuilder, isl_id_to_ast_expr *Indexes)

There are two main differences:

1) I don't use a isl_ast_build *Build in the constructor. I always use
isl_ast_build_from_context get the ast_build whenever I need it.

2) I use a isl_id_to_ast_expr to input the new access ast_expr to
BlockGenerator. (Maybe I should use a setNewAccessExpr method rather than
put it in the constructor.) Because in my project, the optimizer (PPCG)
generate new access expr and store it in a isl_id_to_ast_expr. Then we can
use the following codes to get the new access ast_expr:

  isl_ast_expr *AccessExpr = isl_id_to_ast_expr_get(Indexes,
Access.getBaseAddrID());

  isl_ast_expr *IndexExpr = isl_ast_expr_get_op_arg(AccessExpr, 1);

  Value *Index = ExprBuilder->create(IndexExpr);

this getBaseAddrId() is just the one you introduced in your first patch.
And I agree with Tobias that naming it as getArrayId may be better.


Regards,

Yabin






2014-07-28 19:11 GMT+02:00 Johannes Doerfert <jdoerfert at codeaurora.org>:

> 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140729/c0076d1c/attachment.html>