<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"><head><meta http-equiv=Content-Type content="text/html; charset=utf-8"><meta name=Generator content="Microsoft Word 14 (filtered medium)"><style><!--
/* Font Definitions */
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Tahoma;
panose-1:2 11 6 4 3 5 4 4 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:12.0pt;
font-family:"Times New Roman","serif";}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
span.EmailStyle17
{mso-style-type:personal-reply;
font-family:"Calibri","sans-serif";
color:#1F497D;}
.MsoChpDefault
{mso-style-type:export-only;
font-family:"Calibri","sans-serif";}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]--></head><body lang=EN-US link=blue vlink=purple><div class=WordSection1><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>Hey Yabin,<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>Sorry for my late reply, I have to wrap up my project this week so I’m a bit busy.<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>That’s also the reason I could not wait for your patch, sorry.<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>I have a couple of questions about your approach though:<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>I didn’t know about the isl map you used, What exactly do you map with them?<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>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.<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>Regarding point 2), as you said, if you use the setNewAccessExpr you should get the “magic” for free.<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>Best regards,<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'> Johannes<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>--<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>Johannes Doerfert<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>Employee of Qualcomm Innovation Center, Inc.<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><b><span style='font-size:10.0pt;font-family:"Tahoma","sans-serif"'>From:</span></b><span style='font-size:10.0pt;font-family:"Tahoma","sans-serif"'> Yabin Hu [mailto:yabin.hwu@gmail.com] <br><b>Sent:</b> Tuesday, July 29, 2014 12:36 AM<br><b>To:</b> Johannes Doerfert<br><b>Cc:</b> Johannes Doerfert; Tobias Grosser; llvm-commits@cs.uiuc.edu<br><b>Subject:</b> Re: [Polly] Generating code for changed memory accesses<o:p></o:p></span></p><p class=MsoNormal><o:p> </o:p></p><div><p class=MsoNormal>Hi Johannes,<br><br>Very nice patches! <o:p></o:p></p><div><p class=MsoNormal>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<o:p></o:p></p><div><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'><o:p> </o:p></p><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'><o:p> </o:p></p><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'><o:p> </o:p></p></div></div></div><div><p class=MsoNormal style='margin-bottom:12.0pt'><o:p> </o:p></p><div><p class=MsoNormal>2014-07-28 19:11 GMT+02:00 Johannes Doerfert <<a href="mailto:jdoerfert@codeaurora.org" target="_blank">jdoerfert@codeaurora.org</a>>:<o:p></o:p></p><p class=MsoNormal>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?<o:p></o:p></p><div><p class=MsoNormal style='margin-bottom:12.0pt'><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><o:p></o:p></p></div><div><p class=MsoNormal>-----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)<o:p></o:p></p></div><p class=MsoNormal>> >+{<o:p></o:p></p><div><p class=MsoNormal>> >+ 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>> >+<o:p></o:p></p></div><p class=MsoNormal>> >+ SmallVector<Value *, 4> IndexArray; for (int i = 1; i <<o:p></o:p></p><div><div><p class=MsoNormal style='margin-bottom:12.0pt'>> >+ 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<o:p></o:p></p></div></div></div><p class=MsoNormal><o:p> </o:p></p></div></div></body></html>