[polly] r248603 - Use per-Purpose overloads for MemoryAccess creation

Sat Sep 26 09:43:36 PDT 2015

> Could you also refer to r248616 and see whether the naming makes more
> sense in combination?
In r248616 you distinguish SCALAR, PHI and EXPLICIT, right?
Here "implicit" is gone but "explicit" stays (which could/should be MEMORY).

> 2015-09-26 17:24 GMT+02:00 Johannes Doerfert <doerfert at cs.uni-saarland.de>:
> >> 2015-09-26 1:46 GMT+02:00 Johannes Doerfert <doerfert at cs.uni-saarland.de>:
> >>
> >> >> +  void addExplicitAccess(Instruction *MemAccInst, MemoryAccess::AccessType Type,
> >> >> +                         Value *BaseAddress, unsigned ElemBytes, bool IsAffine,
> >> >> +                         ArrayRef<const SCEV *> Subscripts,
> >> >> +                         ArrayRef<const SCEV *> Sizes, Value *AccessValue);
> >> > Could we call this a MemoryAccess in contrast to ScalarAccesses? Hence,
> >> >   Create an Access for a memory load or store.
> >>
> >> The class is also called MemoryAccess, which applies to both, implicit
> > We should change that too :)
> >
> >> and explicit. IndependentBlocks used to introduce loads/stores for
> >> scalars explicitly into the IR; this is why I don't see the contrast.
> >> Scalars/registers are also memory. Do ScalarAccess include PHINode
> >> emulation accesses?
> >>
> >> What's wrong with calling it "explicit"?
> > Explicit is equal to load/store which is commonly referred to as memory
> > access, though I would just avoid introducing another Polly intern
> > notation for these.
> 
> So it should be called addLoadStoreAccess?
addMemoryAccess, which makes a lot sense after the class is called only
"Access" or "ScopAccess" :)

> "Explicit" is an adjective (not an alternative term for load/store)
> emphasizing that there is a dedicated load/store for the access, in
> contrast to the other kinds that have no dedicated load/store, but we
> still treat it as if.
It does not emphasise that it is a load/store because nobody knows what
a explicit access is without reading the comment. However, load/store
already have a name: __memory__ operations.
  http://llvm.org/docs/LangRef.html#memory-access-and-addressing-operations

> >> >> +  /// @brief Create a MemoryAccess for writing to .s2a memory.
> >> >> +  ///
> >> >> +  /// The access will be created at the @p Value's definition.
> >> >> +  ///
> >> >> +  /// @param Value The value to be written into the .s2a memory.
> >> >> +  void addScalarWriteAccess(Instruction *Value);
> >> > I would avoid .s2a as it is an internal artifact. Maybe just say
> >> > something like:
> >> >   Create a write Access for the definition of a sclar.
> >>
> >> Source could comments should report about internal artifacts.
> > But what is .s2a memory anyway? ".s2a" is just a suffix we use for an
> > alloca name, not a type of memory.
> 
> OK, it should be more precise. I will write another one when we agreed
> on the terms to use.
OK.

> >> As someone who has look at the source a first time, I missed the
> >> information about how those accesses are actually modeled.
> > I don't see the connection between ".s2a" which is __currently__ used in
> > the code generation (a couple of passes after ScopInfo) and the idea of
> > modeling scalar accesses. A comment in code generation might make sense
> > but ".s2a" is an arbitrary suffix that might not live long anyway. In
> > any case, I think when someone reads the ScopInfo documentation she does
> > need to know about the naming of instructions later on. (Btw, in release
> > builds these names are gone anyway and should not even be created.)
> 
> It is still a precise and hence useful term and I think helps a lot
> understanding the code. Avoiding mentioning the ".s2a" alloca would
> require to replace it by something like "the virtual memory for
> representing a register", and hard to distinguish from whatever the
> description of .phiops could be.
> 
> Maybe you have a suggestion?
Sure, my suggestion from the beginning:
  "Create a write Access for the definition of a scalar"

> >> >> +  /// This is for PHINodes using the scalar. It is used when leaving the
> >> >> +  /// incoming block to write to the .phiops location.
> >> >> +  ///
> >> >> +  /// @param Value  The scalar expected to be loaded.
> >> >> +  /// @param User   The PHI node referencing @p Value.
> >> >> +  /// @param UserBB Incoming block for the incoming @p Value.
> >> >> +  void addScalarReadAccess(Value *Value, PHINode *User, BasicBlock *UserBB);
> >> > Maybe we could name this differently to make the purpose clearer, e.g.,
> >> >   addPHIOpReadAccess
> >>
> >> It has the same purpose as the other overload, but with explicit
> >> UserBB instead User->getParent().
> > The comment states the method handles PHI nodes (ignoring the grammar).
> > Is it used for anything else? If not why bother with a general name and a
> > specializing comment instead of a specializing name?
> 
> It handles the reads of scalars. It is a special case of the other
> overload which handles all scalar reads except from PHINodes.
OK, why not say so in the name but only in a note? My initial suggestion
was:
  "addPHIOpReadAccess"
or
  "addPHIOperandReadAccess"
which would make the comment obsolete, wouldn't it?

Also the comment says something about writing ".phiops location" but
isn't this a read access? That should not result in a write to any
location.

> >> >> +  /// @brief Create a MemoryAccess for writing to .phiops memory.
> >> >> +  ///
> >> >> +  /// @param PHI           PHINode under consideration.
> >> >> +  /// @param IncomingBlock Some predecessor block.
> >> >> +  /// @param IncomingValue @p PHI's value when coming from @p IncomingBlock.
> >> >> +  /// @param IsExitBlock   When true, use the .s2a alloca instead. Used for
> >> >> +  ///                      values escaping through a PHINode in the SCoP
> >> >> +  ///                      region's exit block.
> >> >> +  void addPHIWriteAccess(PHINode *PHI, BasicBlock *IncomingBlock,
> >> >> +                         Value *IncomingValue, bool IsExitBlock);
> >> >> +
> >> >> +  /// @brief Create a MemoryAccess for reading from .phiops memory.
> >> >> +  ///
> >> >> +  /// @param PHI PHINode under consideration. READ access will be added here.
> >> >> +  void addPHIReadAccess(PHINode *PHI);
> >> > Again avoid .phiops and make it something like: the phi operand
> >>
> >> What phi operand?
> > Like this:
> > +  /// @brief Create a MemoryAccess for reading the @p PHI operands.
> >
> >>
> >>
> >> >> -  SmallVector<const SCEV *, 4> Subscripts, Sizes;
> >> >> -  Subscripts.push_back(AccessFunction);
> >> >> -  Sizes.push_back(SE->getConstant(ZeroOffset->getType(), Size));
> >> >> +  // FIXME: Size if the number of bytes of an array element, not the number of
> >> >> +  // elements as probably intended here.
> >> > What exactly does this FIXME mean and why is it added?
> >>
> >> The FIXME means that I am pretty sure that Size (byte length of a
> >> single array element) does not match what it is used for (the number
> >> of array elements).
> >>
> >> I did not change it because the value is probably never used and even
> >> if I did not want to introduce behavioral changes.
> >>
> >> It was added because I was rewriting the addMemoryAccess calls and
> >> gathered the data required per use case, finding this mismatch of what
> >> is passed to the function.
> > Let me rephrase: Was it accidentally committed in this revision or not?
> 
> It was on purpose.
> 
> 
> > It also contains a typo (second word).
> 
> Thanks.

-- 

Johannes Doerfert
Researcher / PhD Student

Compiler Design Lab (Prof. Hack)
Saarland University, Computer Science
Building E1.3, Room 4.31

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/20150926/1429283c/attachment.sig>