[polly] r248603 - Use per-Purpose overloads for MemoryAccess creation
Michael Kruse via llvm-commits
llvm-commits at lists.llvm.org
Sat Sep 26 08:12:03 PDT 2015
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
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
What's wrong with calling it "explicit"?
>> + /// @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.
As someone who has look at the source a first time, I missed the
information about how those accesses are actually modeled.
>> + /// 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.,
It has the same purpose as the other overload, but with explicit
UserBB instead User->getParent().
>> + /// @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?
>> - 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.
More information about the llvm-commits