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

[Michael Kruse 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
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"?


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

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.


Michael