<div dir="ltr">Hi Johannes,<div><br></div><div><br></div><div>My comments inline.</div><div><br></div><div>1) For the first one (0001-Update-the-jscop-tests-and-ported-them-to-isl-codege.patch)<br></div><div><br></div><div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Subject: [PATCH 1/3] Update the jscop tests and ported them to isl codegen.<br>
</blockquote><div><br></div><div>"ported" => "port"</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
The updated tests use a different context then the old ones did.<br></blockquote><div><br></div><div>typo, "then" => "than"</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Other then that only their path and the code generation we use<br></blockquote><div><br></div><div>typo, "then" => "than"<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
changed.</blockquote></div><div><br></div><div>2) For the second one (0002-Allow-the-IslExprBuilder-to-generate-access-operatio.patch)</div><div><br></div><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+Value *IslExprBuilder::createOpAccess(isl_ast_expr *Expr) {<br>+ 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 to create a member access.");<br>+<br>+ // TODO: Support for multi-dimensional array<br>
</blockquote><div><br></div>TODO always ends with a dot, right?<br>Otherwise, LGTM.<div><br></div><div>3) For the third patch (0003-Fix-the-modifiable-access-creation.patch) </div><div><br></div><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
using namespace llvm;<br> class ScopStmt;<br>+class MemoryAccess;<br>+class IslExprBuilder;</blockquote></div><div><br></div></div>Use alphabetical order:<br><div>+class IslExprBuilder;</div><div>+class MemoryAccess;<br>
<br></div><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">- /// @param Builder The LLVM-IR Builder used to generate the statement. The<br>
- /// code is generated at the location, the Builder points to.<br>- /// @param Stmt The statement to code generate.<br>- /// @param GlobalMap A map that defines for certain Values referenced from the<br>
- /// original code new Values they should be replaced with.<br>- /// @param P A reference to the pass this function is called from.<br>- /// The pass is needed to update other analysis.<br>
+ /// @param Builder An IR Builder to generate LLVM code.<br>+ /// @param Stmt The statement to generate code for.<br>+ /// @param GlobalMap A map from old values to new versions.<br>+ /// @param P A pass to update other analysis.<br>
+ /// @param Build The AST build with the new schedule.<br>+ /// @param ExprBuilder An expression builder to generate new access functions.</blockquote><div><br></div><div>I would like to just update the format rather than change the comments at the same time. I prefer the original comments.</div>
<div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">--- a/lib/CodeGen/BlockGenerators.cpp<br>
+++ b/lib/CodeGen/BlockGenerators.cpp<br>@@ -15,7 +15,10 @@<br> <br> #include "polly/ScopInfo.h"<br> #include "isl/aff.h"<br>+#include "isl/ast.h"<br>+#include "isl/ast_build.h"<br>
#include "isl/set.h"<br>+#include "polly/CodeGen/IslExprBuilder.h"<br></blockquote><div><br></div><div>Using alphabetical order: put this include down after #include "polly/CodeGen/CodeGeneration.h"</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> #include "polly/CodeGen/BlockGenerators.h"<br>
#include "polly/CodeGen/CodeGeneration.h"<br> #include "polly/Options.h" </blockquote><div>....</div><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+ PWSchedule = isl_pw_multi_aff_from_map(isl_map_reverse(Schedule));<br>+ PWAccRel = isl_pw_multi_aff_from_map(AccRel);<br>+ PWAccRel = isl_pw_multi_aff_pullback_pw_multi_aff(PWAccRel, PWSchedule);<br>+<br>+ Expr = isl_ast_build_access_from_pw_multi_aff(Build, PWAccRel);<br>
+ return ExprBuilder->create(Expr);</blockquote><div><br></div><div>Add a blank line before return statement.</div><div><br></div><div>Very nice patches.</div><div><br></div><div>Regards,</div><div>Yabin <br></div></div>
<div><br></div><div> </div></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">2014-08-02 20:27 GMT+02:00 Johannes Doerfert <span dir="ltr"><<a href="mailto:doerfert@cs.uni-saarland.de" target="_blank">doerfert@cs.uni-saarland.de</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hey Yabin,<br>
<br>
good to hear that the patches work for you.<br>
<br>
Did you only apply these patches or "review" them?<br>
<br>
Best reagards,<br>
Johannes<br>
<div class="HOEnZb"><div class="h5"><br>
<br>
On 08/02, Yabin Hu wrote:<br>
> Hi Johannes,<br>
><br>
> I have rebased my GSoC project totally on top of these three patches and<br>
> the polly trunk, it works well at the moment. Very nice patches!<br>
><br>
> As Tobias has explained, I compute an entirely new ast node and use the<br>
> islcodegeneration to translate it into LLVM IR. That's why I always have<br>
> the context from which I can get the isl_ast_build.<br>
><br>
> Thanks,<br>
> Yabin<br>
><br>
><br>
> 2014-08-02 1:27 GMT+02:00 Johannes Doerfert <<a href="mailto:jdoerfert@codeaurora.org">jdoerfert@codeaurora.org</a>>:<br>
><br>
> > Please see the attached patches<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<br>
> > by The Linux Foundation<br>
> ><br>
> ><br>
> > -----Original Message-----<br>
> > From: Tobias Grosser [mailto:<a href="mailto:tobias@grosser.es">tobias@grosser.es</a>]<br>
> > Sent: Thursday, July 31, 2014 3:48 PM<br>
> > To: Johannes Doerfert<br>
> > Cc: <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 01/08/2014 00:42, Johannes Doerfert wrote:<br>
> > >>> To your problem: Why would they fail? I remember you mentioned<br>
> > something about a different context being expected by isl. What is the<br>
> > minimal set of changes necessary to move the tests to the<br>
> > IslCodegeneration? Just changes to the context fields in the .json files?<br>
> > > There are two problems, but if you just want to keep these tests you<br>
> > just need to address the first one:<br>
> > > 1) Change the context from { [] } to { : }<br>
> > > 2) As soon as scev codegen is turned on and we have new access<br>
> > functions we crash.<br>
> ><br>
> > The second already exists in the current code, so it is not really a<br>
> > regression (You can use an explicitly disable scev code generation if this<br>
> > helps to keep your internal tests running).<br>
> ><br>
> > The first one seems easy to do. Could you move the tests and mention in<br>
> > the commit message that this is the only change that has been applied to<br>
> > these files.<br>
> ><br>
> > The remaining patch should then not need to change these files, no? If<br>
> > this is the case, you can mention in the commit messages that the change is<br>
> > tested by existing test cases (maybe point to them).<br>
> ><br>
> > Cheers,<br>
> > Tobias<br>
> ><br>
> ><br>
> > _______________________________________________<br>
> > llvm-commits mailing list<br>
> > <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
> ><br>
> ><br>
<br>
</div></div><span class="HOEnZb"><font color="#888888">--<br>
<br>
Johannes Doerfert<br>
Researcher / PhD Student<br>
<br>
Compiler Design Lab (Prof. Hack)<br>
Saarland University, Computer Science<br>
Building E1.3, Room 4.26<br>
<br>
Tel. <a href="tel:%2B49%20%280%29681%20302-57521" value="+4968130257521">+49 (0)681 302-57521</a> : <a href="mailto:doerfert@cs.uni-saarland.de">doerfert@cs.uni-saarland.de</a><br>
Fax. <a href="tel:%2B49%20%280%29681%20302-3065" value="+496813023065">+49 (0)681 302-3065</a> : <a href="http://www.cdl.uni-saarland.de/people/doerfert" target="_blank">http://www.cdl.uni-saarland.de/people/doerfert</a><br>
</font></span></blockquote></div><br></div>