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