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

[Michael Kruse via llvm-commits]

2015-09-26 18:43 GMT+02:00 Johannes Doerfert <doerfert at cs.uni-saarland.de>:
>> 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).

bool isExplicit() const { return Origin == EXPLICIT; }
bool isImplicit() const { return !isExplicit(); }

"implicit" was never even mentioned in this commit (r248603).

Don't you think it would be paradoxical to instantiate a class called
MemoryAccess with something else than MEMORY?

Just to illustrate why the naming scheme is consistent:

MemoryOrigin::EXPLICIT
addExplicitAccess()

MemoryOrigin::SCALAR
addScalarWriteAccess()
addScalarReadAccess()

MemoryOrigin::PHI
addPHIWriteAccess()
addPHIReadAccess()

There will be a fourth type, "MAPPED" which is implicit (replacing
some SCALARs/PHIs), but accesses "real" memory in arrays.



>> 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" :)

If you think that the used terms are wrong even before this commit, it
is not this commit's fault to be consistent what has been there before
(Also see all the comments referring to memory). You can suggest a
renaming independently if you want.

AFAIU the discussion is about that whether def/uses of registers and
phi nodes can be called "memory" accesses too. And IMHO we handle them
as such, so we should call them accordingly. The entire point of this
is that the polyhedral model only knows array (memory) accesses.

This might lead to the idea that we use the term "ArrayAccess" instead
of "Explicit". Unfortunately this isn't consistent with the MAPPED
type, which are also array accesses.


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

That's why there are comments. Is "addExplicitMemoryAccess" better? I
could live with that.

>From the function name only, a reader cannot even deduce where the
access is added to.


> However, load/store
> already have a name: __memory__ operations.
>   http://llvm.org/docs/LangRef.html#memory-access-and-addressing-operations

I never questioned that loads/stores are memory operations.


>> >> 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"

I mean, a general term describing how scalars and phis are modeled. It
is important to me to mention that phis are modeled independently from
scalars, and how a definition can be a write.

The more interesting case is the comment for
MemoryAccess::AccessOrigin. If you can rewrite it in such a way that
it is more understandable but conveys at least the same information, I
will be convinced and rewrite the rest by myself. Tobias can be judge.


>> 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
> was:
>   "addPHIOpReadAccess"
> or
>   "addPHIOperandReadAccess"
> 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
> location.

Please read the whole comment which unfortunately has been cut of in
my reply. The additional remarks describes this function's use case,
not what it does.


Michael