[Polly] Generating code for changed memory accesses
Johannes Doerfert
doerfert at cs.uni-saarland.de
Tue Jul 29 01:44:28 PDT 2014
First part:
r214165, r214166, r214168, r214169
> >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
-------------- 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/20140729/be8ba2e9/attachment.sig>
More information about the llvm-commits
mailing list