[LLVMdev] [polly] ISL vector code generation

[Tobias Grosser]

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