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

[Johannes Doerfert]

On 08/11, Tobias Grosser wrote:
> 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.
Granted.

> > 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.
How is that going to work?

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

> Tobias

-- 

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 --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 213 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140811/7fcea6ef/attachment.sig>