[Polly][PATCH 5/8] Allow the IslExprBuilder to build address of expressions

[Tobias Grosser]

On 11/08/2014 09:36, Johannes Doerfert wrote:
> On 08/11, Tobias Grosser wrote:
>> On 10/08/2014 09:50, Johannes Doerfert wrote:
>>> ---
>>>   include/polly/CodeGen/IslExprBuilder.h |  1 +
>>>   lib/CodeGen/IslExprBuilder.cpp         | 26 ++++++++++++++++++++++++++
>>>   2 files changed, 27 insertions(+)
>>>
>>> diff --git a/include/polly/CodeGen/IslExprBuilder.h b/include/polly/CodeGen/IslExprBuilder.h
>>> index c87cd57..39e7044 100644
>>> --- a/include/polly/CodeGen/IslExprBuilder.h
>>> +++ b/include/polly/CodeGen/IslExprBuilder.h
>>> @@ -128,6 +128,7 @@ private:
>>>     llvm::Value *createOpBoolean(__isl_take isl_ast_expr *Expr);
>>>     llvm::Value *createId(__isl_take isl_ast_expr *Expr);
>>>     llvm::Value *createInt(__isl_take isl_ast_expr *Expr);
>>> +  llvm::Value *createOpAddrOf(__isl_take isl_ast_expr *Expr);
>>>   };
>>>   }
>>>
>>> diff --git a/lib/CodeGen/IslExprBuilder.cpp b/lib/CodeGen/IslExprBuilder.cpp
>>> index b9bdabe..5eeab39 100644
>>> --- a/lib/CodeGen/IslExprBuilder.cpp
>>> +++ b/lib/CodeGen/IslExprBuilder.cpp
>>> @@ -389,11 +389,37 @@ Value *IslExprBuilder::createOp(__isl_take isl_ast_expr *Expr) {
>>>     case isl_ast_op_ge:
>>>     case isl_ast_op_gt:
>>>       return createOpICmp(Expr);
>>> +  case isl_ast_op_addr_of:
>>> +    return createOpAddrOf(Expr);
>>
>> I would prefer AddressOf
> Sure.
>
>>>     }
>>>
>>>     llvm_unreachable("Unsupported isl_ast_expr_op kind.");
>>>   }
>>>
>>> +Value *IslExprBuilder::createOpAddrOf(__isl_take isl_ast_expr *Expr) {
>>> +  assert(isl_ast_expr_get_type(Expr) == isl_ast_expr_op &&
>>> +         "Expected an isl_ast_expr_op expression.");
>>> +  assert(isl_ast_expr_get_op_n_arg(Expr) == 1 && "Address of should be unary.");
>>> +
>>> +  isl_ast_expr *Op = isl_ast_expr_get_op_arg(Expr, 0);
>>> +  assert(isl_ast_expr_get_type(Op) == isl_ast_expr_op &&
>>> +         "Expected address of operator to be an isl_ast_expr_op expression.");
>>> +  assert(isl_ast_expr_get_op_type(Op) == isl_ast_op_access &&
>>> +         "Expected address of operator to be an access expression.");
>>> +
>>> +  Value *V = create(Op);
>>> +
>>> +  LoadInst *LI = dyn_cast<LoadInst>(V);
>>> +  assert(LI && "Address of operand should result in a load.");
>>> +
>>> +  V = LI->getPointerOperand();
>>> +  LI->eraseFromParent();
>>
>> This is a little hackish. I would prefer that we factor out the code that
>> computes the address of an access and that we just call it from both, this
>> location and the location where we generate the load.
> This is not as hackish as it looks like. The reason is the restriction on
> the new isl_ast_expr address_of operator. So far it is only applicable to access
> operations (I can add a comment as the assert seems not to explain it
> very well).

Still, adding an instruction to just remove it a second later seems hackish.

 > In the future it might be applicable to more but the "types"
> still have to match, thus in our code generation the operand should
> result in a load/store (the store case is missing at the moment though).
> If you want me to factor it out, sure...
>
>> Also, I am surprised that create(Op) actually returns a load instruction? My
>> understanding is that we do not create load/stores for access expressions,
>> but that we only create the ptr value?
> I change the behaviour of createOpAccess to generate the load and the
> reason is pretty simple:
>    A isl_ast_expr which dumps like MemRef_sum[0] or MemRef_A[i] should
>    not result in a LLVM-Value representing &MemRef_sum[0] or
>    &MemRef_A[i]. What we did before for isl ast access expressions was
>    actually implementing the address of the access not the actual
>    access.

Right. You should mention such things in your commit message. They are 
very helpful to understand what you are doing.

Nevertheless, you change just makes the behaviour unintuitive in a 
different way.

As proposed before, I would just assert if we see an isl_ast_access that
is not surrounded by the address_of instruction.

And the address_of instruction could directly compute the address (using 
a subfunction), without having to create a load that is later removed.

Tobias