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

[Johannes Doerfert via llvm-commits]

> 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.

> >> +  /// @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.

> 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.)

> >> +  /// 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?

> >> +  /// @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 also contains a typo (second word).


Cheers,
  Johannes

-- 

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/80f5591c/attachment.sig>