[polly] r248603 - Use per-Purpose overloads for MemoryAccess creation
Johannes Doerfert via llvm-commits
llvm-commits at lists.llvm.org
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.
> >> >> + /// @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.
> >> 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
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
> >> >> + /// @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).
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
Size: 213 bytes
Desc: not available
More information about the llvm-commits