On 12/18/2012 12:18 AM, Sebastian Pop wrote:> Hi,
>
> thanks to Tobias for doing most of the work to get the vector code
generation
> working with ISL's code generation. Attached are two patches to port
the vector
> code generation part of the testsuite from Cloog/CodeGen to Isl/CodeGen.
>
> Tobi, do you want to commit these two patches separately, or you want me to
> combine them, as the second patch is needed to make the testsuite pass?
Hi Sebastian,
I think it would be best if we first commit the second patch, but
without the isl code generation changes. This is mainly a refactoring
that establishes a proper interface for the isStride... functions. The
set that we passed in until now was very CLooG specific. Now we have a
proper interface and only need to adapt the CLooG output slightly to
match this interface. I added some comments inline regarding this patch.
The remaining parts can then be merged to a single patch that adds
vectorization support.
The first patch looks good, but needs still the stride-stuff from the
second patch.
The second patch still needs some love. Mainly documentation
and some little stylistic changes. Here the comments:
>
> int getVectorWidth();
>
> diff --git a/include/polly/ScopInfo.h b/include/polly/ScopInfo.h
> index a6cf997..0d9ba1e 100755
> --- a/include/polly/ScopInfo.h
> +++ b/include/polly/ScopInfo.h
> @@ -150,18 +150,18 @@ public:
>
> /// @brief Get the stride of this memory access in the specified domain
> /// subset.
> - isl_set *getStride(__isl_take const isl_set *domainSubset) const;
> + isl_set *getStride(__isl_take const isl_map *SubSchedule) const;
I dislike the name SubSchedule. Maybe we can use PartialSchedule or just
Schedule. Also, we should document what kind of map is expected. Meaning
a map from the statement to a schedule where the innermost dimension of
the schedule is the dimension for which we want to get the stride.
> diff --git a/lib/CodeGen/CodeGeneration.cpp
b/lib/CodeGen/CodeGeneration.cpp
> index 737f3b2..ccfcd04 100644
> --- a/lib/CodeGen/CodeGeneration.cpp
> +++ b/lib/CodeGen/CodeGeneration.cpp
> @@ -422,7 +422,16 @@ void ClastStmtCodeGen::codegen(const clast_user_stmt
*u,
> }
> }
>
> - VectorBlockGenerator::generate(Builder, *Statement, VectorMap, Domain,
P);
> + isl_map *Schedule = Statement->getScattering();
> + int ScheduledDimensions = isl_set_dim(Domain, isl_dim_set);
> + int UnscheduledDimensions = isl_map_dim(Schedule, isl_dim_out)
> + - ScheduledDimensions;
> +
> + Schedule = isl_map_project_out(Schedule, isl_dim_out,
ScheduledDimensions,
> + UnscheduledDimensions);
> + VectorBlockGenerator::generate(Builder, *Statement, VectorMap, Schedule,
P);
Can we move this change into a separate function. This will allow us to
document that this function takes the cloog specific domain and
translates it into a map Statement -> PartialSchedule, where the partial
schedule for all dimensions that have been code generated up to this point.
> + isl_map_free(Schedule);
> }
>
> void ClastStmtCodeGen::codegen(const clast_block *b) {
> diff --git a/lib/CodeGen/IslCodeGeneration.cpp
b/lib/CodeGen/IslCodeGeneration.cpp
> index 431b960..c83e4b7 100644
> --- a/lib/CodeGen/IslCodeGeneration.cpp
> +++ b/lib/CodeGen/IslCodeGeneration.cpp
> @@ -596,7 +596,7 @@ private:
> void createIf(__isl_take isl_ast_node *If);
> void createUserVector(__isl_take isl_ast_node *User,
> std::vector<Value*> &IVS, __isl_take
isl_id *IteratorID,
> - isl_set *Domain);
> + __isl_take isl_union_map *Domain);
The name should be consistent in function definition and declaration.
> void createUser(__isl_take isl_ast_node *User);
> void createBlock(__isl_take isl_ast_node *Block);
> };
> @@ -671,7 +671,7 @@ unsigned
IslNodeBuilder::getNumberOfIterations(__isl_keep isl_ast_node *For) {
> void IslNodeBuilder::createUserVector(__isl_take isl_ast_node *User,
> std::vector<Value*> &IVS,
> __isl_take isl_id *IteratorID,
> - __isl_keep isl_set *Domain) {
> + __isl_take isl_union_map
*SubSchedule) {
I am not sure if 'SubSchedule' is very descriptive. PartialSchedule
maybe. Also, we may want to add somewhere a comment that this is the
schedule reaching up to the depth of the current statement.
> isl_id *Annotation = isl_ast_node_get_annotation(User);
> assert(Annotation && "Vector user statement is not
annotated");
>
> @@ -682,11 +682,17 @@ void IslNodeBuilder::createUserVector(__isl_take
isl_ast_node *User,
> ScopStmt *Stmt = (ScopStmt *) isl_id_get_user(Id);
> VectorValueMapT VectorMap(IVS.size());
>
> + SubSchedule = isl_union_map_intersect_domain(SubSchedule,
> + isl_union_set_from_set(Stmt->getDomain()));
You can use a variable 'Domain' to get rid of this ugly line break. ;-)
> + isl_map *Schedule = isl_map_from_union_map(SubSchedule);
> +
> createSubstitutionsVector(isl_pw_multi_aff_copy(Info->PMA),
> isl_ast_build_copy(Info->Context),
> Stmt, VectorMap, IVS, IteratorID);
> - VectorBlockGenerator::generate(Builder, *Stmt, VectorMap, Domain, P);
> + VectorBlockGenerator::generate(Builder, *Stmt, VectorMap, Schedule, P);
After having addressed the comments, you can commit the patches. I will
look at them again post-commit.
Cheers
Tobias