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

[Michael Kruse via llvm-commits]

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

Thanks.