[Polly] Generating code for changed memory accesses

Wed Jul 30 09:37:43 PDT 2014

Hey Yabin,

Sorry for my late reply, I have to wrap up my project this week so I’m a bit busy.

That’s also the reason I could not wait for your patch, sorry.

I have a couple of questions about your approach though:

I didn’t know about the isl map  you used, What exactly do you map with them?

Regarding point 1), how do you get a isl_ast_build with the correct schedule? If I build it from context (which is a set) it doesn’t have any meaningful schedule.

Regarding point 2), as you said, if you use the setNewAccessExpr you should get the “magic” for free.

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

From: Yabin Hu [mailto:yabin.hwu at gmail.com] 
Sent: Tuesday, July 29, 2014 12:36 AM
To: Johannes Doerfert
Cc: Johannes Doerfert; Tobias Grosser; llvm-commits at cs.uiuc.edu
Subject: Re: [Polly] Generating code for changed memory accesses

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/20140730/13a56488/attachment.html>