[Polly] Generating code for changed memory accesses

[Tobias Grosser]

On 28/07/2014 10:17, Johannes Doerfert wrote:
> On 07/25, Tobias Grosser wrote:
>>> > >[This also breaks the little functionality the jscop-cloog interface
>>> > >offered, but I don't think that should stop us at this point.]
>> >
>> >I am OK with breaking CLooG as long as we move the test cases such that
>> >we both preserve test coverage and do not cause failing test cases.
> I moved & updated them to work with the isl codegen backend.

Nice.

>>> > >+Value *IslExprBuilder::createOpAccess(__isl_take 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 in an n-ary operation");
>>> > >+
>>> > >+  Value *Base = create(isl_ast_expr_get_op_arg(Expr, 0));
>>> > >+
>>> > >+  SmallVector<Value *, 4> IndexArray;
>>> > >+  for (int i = 1; i < isl_ast_expr_get_op_n_arg(Expr); ++i) {
>>> > >+    Value *OpV = create(isl_ast_expr_get_op_arg(Expr, i));
>>> > >+    OpV = Builder.CreateSExt(OpV, getType(Expr));
>>> > >+    IndexArray.push_back(OpV);
>>> > >+  }
>> >
>> >I would prefer to just assert in case we have more than one array
>> >dimensions and to remove this loop. As you state above, we most likely
>> >don't want to push those values into IndexArray, but instead compute the
>> >linearized access ourselves. Instead of committing code that is
>> >currently unused and likely to change later, I prefer to state the
>> >limitation and only commit the code later.

> 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:
>
> int A[1024]
>
> void f()
>    for i
>       A[i]
>
> Now the gep needs two indices [0, i].
> 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.

Interesting. I am not yet sure either, but it is good that you at least 
did not reintroduce the full loop. (I would like to look into this a 
little later, but would like to get most of this patch already out of 
the way).

>>> > >+  // TODO: In case of multiple dimensions we need to compute the offset ourselfs
>> >ourselves
>> >
>>> > >+  //       since the Base is likely a simple pointer. To do so we need access to
>>> > >+  //       the memory access associated with the "original base" as it stores
>>> > >+  //       the delinearization information.
>> >
>> >I do not understand the second sentence.
>> >
>> >[I would just leave out the todo or shorten it to 'Support for
>> >multi-dimensional arrays]
> Done.
>
>> >After addressing these comments, this patch is good to go.
> 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).

What does it mean on by default? This is not clear to me and I am afraid 
that we break non-affine accesses which we can not code generate with isl.

If possible, I would prefer to re-discuss this after the cleanup patches 
are out of the way.

> 0001-Refactor-Update-the-MemoryAccess.patch

Great that you split out this patch.

>  From 89fcf4f8a9e3ffc1800b80def0da9e8d2c567e8e Mon Sep 17 00:00:00 2001
> From: Johannes Doerfert<doerfert at cs.uni-saarland.de>
> Date: Mon, 28 Jul 2014 01:04:57 -0700
> Subject: [PATCH 1/2] [Refactor] Update the MemoryAccess

What does this mean? Maybe just

'Some MemoryAccess cleanups'?

>    + Remove an unused constructor

This change is entirely unrelated and obvious. No need for patch reviews 
here. You could have committed it without review.

>    + Use a non const base address llvm::Value pointer
 >    + Remove base address const_casts from the backend

OK.

Also, thanks for the documentation updates.

You could include those changes as well in the cleanup commit.

>    + Add the base address isl_id

This is a tiny new addition, which I would prefer to have either in a 
separate patch or even better, in the patch that actually uses it. As it 
is really tiny you can possibly also leave it in this patch.

> +  /// @brief The isl_id of the base address (output id of the access relation).
> +  isl_id *BaseAddrId;

I would call this getArrayID().

The reasoning is that A[] is not only the base address, but also the 
name of the original array and we will in the future need more 
information about this array (element types, sizes, ...). Hence, my 
secret plan is to have this isl_id point to a polly::Array at some 
point, which contains besides other things also a base address.

> diff --git a/lib/Analysis/ScopInfo.cpp b/lib/Analysis/ScopInfo.cpp
> index 2028f76..d0589d3 100644
> --- a/lib/Analysis/ScopInfo.cpp
> +++ b/lib/Analysis/ScopInfo.cpp
> @@ -305,10 +305,13 @@ static MemoryAccess::ReductionType getReductionType(const BinaryOperator *BinOp,
>   //===----------------------------------------------------------------------===//
>
>   MemoryAccess::~MemoryAccess() {
> +  isl_id_free(BaseAddrId);

This is not needed (see next comment).

>     isl_map_free(AccessRelation);
>     isl_map_free(newAccessRelation);
>   }
>
> +isl_id *MemoryAccess::getBaseAddrId() const { return isl_id_copy(BaseAddrId); }

I would prefer for you to just use:

isl_map_get_tuple_id(AccessRelation, isl_dim_out);

No need to redundantly store an isl_id in MemoryAccess.

>     return isl_basic_map_from_domain_and_range(
> @@ -399,8 +402,10 @@ MemoryAccess::MemoryAccess(const IRAccess &Access, const Instruction *AccInst,
>                              ScopStmt *Statement)
>       : Statement(Statement), Inst(AccInst), newAccessRelation(nullptr) {
>
> +  isl_ctx *ctx = Statement->getIslCtx();

LLVM variables start with uppercase: Ctx.

(I understand that this is confusing coming from isl, but it becomes 
even more confusing if we start mixing the two styles).

>     BaseAddr = Access.getBase();
>     BaseName = getIslCompatibleName("MemRef_", getBaseAddr(), "");
> +  BaseAddrId = isl_id_alloc(ctx, getBaseName().c_str(), nullptr);
>
>     if (!Access.isAffine()) {
>       // We overapproximate non-affine accesses with a possible access to the
> @@ -414,8 +419,7 @@ MemoryAccess::MemoryAccess(const IRAccess &Access, const Instruction *AccInst,
>
>     Type = Access.isRead() ? READ : MUST_WRITE;
>
> -  isl_space *Space = isl_space_alloc(Statement->getIslCtx(), 0,
> -                                     Statement->getNumIterators(), 0);
> +  isl_space *Space = isl_space_alloc(ctx, 0, Statement->getNumIterators(), 0);
>     AccessRelation = isl_map_universe(Space);
>
>     for (int i = 0, Size = Access.Subscripts.size(); i < Size; ++i) {
> @@ -432,8 +436,7 @@ MemoryAccess::MemoryAccess(const IRAccess &Access, const Instruction *AccInst,
>         // each other in memory. By this division we make this characteristic
>         // obvious again.
>         isl_val *v;
> -      v = isl_val_int_from_si(isl_pw_aff_get_ctx(Affine),
> -                              Access.getElemSizeInBytes());
> +      v = isl_val_int_from_si(ctx, Access.getElemSizeInBytes());
>         Affine = isl_pw_aff_scale_down_val(Affine, v);
>       }
>
> @@ -445,10 +448,11 @@ MemoryAccess::MemoryAccess(const IRAccess &Access, const Instruction *AccInst,
>     Space = Statement->getDomainSpace();
>     AccessRelation = isl_map_set_tuple_id(
>         AccessRelation, isl_dim_in, isl_space_get_tuple_id(Space, isl_dim_set));
> -  isl_space_free(Space);
> -  AccessRelation = isl_map_set_tuple_name(AccessRelation, isl_dim_out,
> -                                          getBaseName().c_str());
> +  AccessRelation =
> +      isl_map_set_tuple_id(AccessRelation, isl_dim_out, getBaseAddrId());
> +
>     assumeNoOutOfBound(Access);
> +  isl_space_free(Space);
>   }
>
>   void MemoryAccess::realignParams() {
> @@ -456,16 +460,6 @@ void MemoryAccess::realignParams() {
>     AccessRelation = isl_map_align_params(AccessRelation, ParamSpace);
>   }
>
> -MemoryAccess::MemoryAccess(const Value *BaseAddress, ScopStmt *Statement)
> -    : Type(READ), BaseAddr(BaseAddress), Statement(Statement),
> -      newAccessRelation(nullptr) {
> -
> -  isl_basic_map *BasicAccessMap = createBasicAccessMap(Statement);
> -  AccessRelation = isl_map_from_basic_map(BasicAccessMap);
> -  isl_space *ParamSpace = Statement->getParent()->getParamSpace();
> -  AccessRelation = isl_map_align_params(AccessRelation, ParamSpace);
> -}
> -

Nice.

>   raw_ostream &polly::operator<<(raw_ostream &OS,
>                                  MemoryAccess::ReductionType RT) {
>     switch (RT) {
> diff --git a/lib/CodeGen/BlockGenerators.cpp b/lib/CodeGen/BlockGenerators.cpp
> index 3cfdb4b..7284e24 100644
> --- a/lib/CodeGen/BlockGenerators.cpp
> +++ b/lib/CodeGen/BlockGenerators.cpp
> @@ -307,7 +307,7 @@ Value *BlockGenerator::generateLocationAccessed(const Instruction *Inst,
>       NewPointer =
>           getNewValue(Pointer, BBMap, GlobalMap, LTS, getLoopForInst(Inst));
>     } else {
> -    Value *BaseAddress = const_cast<Value *>(Access.getBaseAddr());
> +    Value *BaseAddress = Access.getBaseAddr();

Nice.

>       NewPointer = getNewAccessOperand(NewAccessRelation, BaseAddress, BBMap,
>                                        GlobalMap, LTS, getLoopForInst(Inst));
>     }
> diff --git a/lib/CodeGen/CodeGeneration.cpp b/lib/CodeGen/CodeGeneration.cpp
> index 56b2ab3..6b61927 100644
> --- a/lib/CodeGen/CodeGeneration.cpp
> +++ b/lib/CodeGen/CodeGeneration.cpp
> @@ -667,7 +667,7 @@ SetVector<Value *> ClastStmtCodeGen::getGPUValues(unsigned &OutputBytes) {
>     // Record the memory reference base addresses.
>     for (ScopStmt *Stmt : *S) {
>       for (MemoryAccess *MA : *Stmt) {
> -      Value *BaseAddr = const_cast<Value *>(MA->getBaseAddr());
> +      Value *BaseAddr = MA->getBaseAddr();
Nice.


> 0002-Expose-the-IslExprBuilder-and-fix-the-modifiable-acc.patch
>
>
>  From 4f160d8c861986ed66e7f06c517087cd895e7e7b Mon Sep 17 00:00:00 2001
> From: Johannes Doerfert<jdoerfert at codeaurora.org>
> Date: Wed, 23 Jul 2014 17:28:17 -0700
> Subject: [PATCH 2/2] Expose the IslExprBuilder and fix the modifiable access
>   creation

This patch becomes huge, so that it is both difficult to review, but 
more importantly that people looking into history will have a hard time 
to understand it e.g. when analysing bugs.

Would it be difficult to separate out the following two changes out:

1) >    + Move IslExprBuilder declaration to an own header file.

(If you like you could also move the IslExprBuilder into its own .cpp file)

2) The move of the MemAccess test files from Cloog to isl

Those two patches contain the _bulk_ of the code and are very unlikely 
to introduce errors as they are only copying codes and do not
contain modifications.

Afterwards the patch should be a lot smaller and easier to look through.

Cheers,
Tobias