[polly] r248603 - Use per-Purpose overloads for MemoryAccess creation
Michael Kruse via llvm-commits
llvm-commits at lists.llvm.org
Sat Sep 26 09:14:04 PDT 2015
Could you also refer to r248616 and see whether the naming makes more
sense in combination?
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?
"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.
>> >> + /// @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?
>> >> + /// 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.
>> >> + /// @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).
More information about the llvm-commits