<div dir="ltr">Hi Johannes,<br><br>Very nice patches! <div>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.<br>
<br>BlockGenerator::BlockGenerator(PollyIRBuilder &B, ScopStmt &Stmt, Pass *P, IslExprBuilder *ExprBuilder, isl_id_to_ast_expr *Indexes)<br><br>There are two main differences:<br><br>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.<br>
<br>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:<br>
<br> isl_ast_expr *AccessExpr = isl_id_to_ast_expr_get(Indexes, Access.getBaseAddrID());<br><br> isl_ast_expr *IndexExpr = isl_ast_expr_get_op_arg(AccessExpr, 1);<br><br> Value *Index = ExprBuilder->create(IndexExpr);<br>
<br>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.<br><br><br>Regards,<br><br>Yabin<br><div><p class=""><br></p><p class=""><br>
</p><p class=""><br></p></div></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">2014-07-28 19:11 GMT+02:00 Johannes Doerfert <span dir="ltr"><<a href="mailto:jdoerfert@codeaurora.org" target="_blank">jdoerfert@codeaurora.org</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hey,<br>
<br>
The last version of the patch still has some flaws. One of which is caused by tiling/pre-vectorization.<br>
Once the optimizer changes the number of dimensions, the access relation does not match the schedule anymore.<br>
Fixing this is "easy" as we only need to apply the scattering of the schedule before we eliminate constant dimensions.<br>
However, we still have a problem if the "AST-codegen" will eliminate dimensions<br>
And this iteration is not fixed in our access relation after applying the schedule (e.g. when we prevectorize a loop with 3 iterations).<br>
<br>
We somehow need to perform the same "transformation" as the AST-codegen to get an access relation which matches our actual schedule.<br>
Any thoughts on how to do that?<br>
<div class=""><br>
Best regards,<br>
Johannes<br>
<br>
--<br>
<br>
Johannes Doerfert<br>
Employee of Qualcomm Innovation Center, Inc.<br>
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation<br>
<br>
<br>
</div><div class="">-----Original Message-----<br>
From: Johannes Doerfert [mailto:<a href="mailto:doerfert@cs.uni-saarland.de">doerfert@cs.uni-saarland.de</a>]<br>
Sent: Monday, July 28, 2014 1:18 AM<br>
To: Tobias Grosser<br>
Cc: Johannes Doerfert; 'Yabin Hu'; <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
Subject: Re: [Polly] Generating code for changed memory accesses<br>
<br>
On 07/25, Tobias Grosser wrote:<br>
> >[This also breaks the little functionality the jscop-cloog interface<br>
> >offered, but I don't think that should stop us at this point.]<br>
><br>
> I am OK with breaking CLooG as long as we move the test cases such<br>
> that we both preserve test coverage and do not cause failing test cases.<br>
I moved & updated them to work with the isl codegen backend.<br>
<br>
> >+Value *IslExprBuilder::createOpAccess(__isl_take isl_ast_expr *Expr)<br>
</div>> >+{<br>
<div class="">> >+ assert(isl_ast_expr_get_type(Expr) == isl_ast_expr_op &&<br>
> >+ "isl ast expression not of type isl_ast_op");<br>
> >+ assert(isl_ast_expr_get_op_type(Expr) == isl_ast_op_access &&<br>
> >+ "not an access isl ast expression");<br>
> >+ assert(isl_ast_expr_get_op_n_arg(Expr) >= 2 &&<br>
> >+ "We need at least two operands in an n-ary operation");<br>
> >+<br>
> >+ Value *Base = create(isl_ast_expr_get_op_arg(Expr, 0));<br>
> >+<br>
</div>> >+ SmallVector<Value *, 4> IndexArray; for (int i = 1; i <<br>
<div class="HOEnZb"><div class="h5">> >+ isl_ast_expr_get_op_n_arg(Expr); ++i) {<br>
> >+ Value *OpV = create(isl_ast_expr_get_op_arg(Expr, i));<br>
> >+ OpV = Builder.CreateSExt(OpV, getType(Expr));<br>
> >+ IndexArray.push_back(OpV);<br>
> >+ }<br>
><br>
> I would prefer to just assert in case we have more than one array<br>
> dimensions and to remove this loop. As you state above, we most likely<br>
> don't want to push those values into IndexArray, but instead compute<br>
> the linearized access ourselves. Instead of committing code that is<br>
> currently unused and likely to change later, I prefer to state the<br>
> limitation and only commit the code later.<br>
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:<br>
<br>
int A[1024]<br>
<br>
void f()<br>
for i<br>
A[i]<br>
<br>
Now the gep needs two indices [0, i].<br>
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.<br>
<br>
> >+ // TODO: In case of multiple dimensions we need to compute the<br>
> >+ offset ourselfs<br>
> ourselves<br>
><br>
> >+ // since the Base is likely a simple pointer. To do so we need access to<br>
> >+ // the memory access associated with the "original base" as it stores<br>
> >+ // the delinearization information.<br>
><br>
> I do not understand the second sentence.<br>
><br>
> [I would just leave out the todo or shorten it to 'Support for<br>
> multi-dimensional arrays]<br>
Done.<br>
<br>
> After addressing these comments, this patch is good to go.<br>
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).<br>
<br>
Best regards,<br>
Johannes<br>
<br>
</div></div></blockquote></div><br></div>