[PATCH] D15307: ScopInfo: Harmonize the different ARRAY_KINDs

[Tobias Grosser via llvm-commits]

grosser added a comment.

In http://reviews.llvm.org/D15307#305845, @Meinersbur wrote:

> I welcome any consistent naming scheme.
>
> MemoryAccess::print still prints "[SCALAR: 0]" or "[SCALAR: 1]". It should maybe print the kind instead, although this requires a lot of unit tests to change.


Scalar:0 and Scalar: 1 is technically not wrong, but clearly we code print more precise  information. However, this seems to be a separate patch (especially due to the unit-test noise it will cause).

> KIND_VALUE fits because it originates from an llvm::Value, analogously to a KIND_PHI originating from an llvm:PHINode. The reason I did not name like this in my first commit is that we have WRITE MemoryAccesses to such "values", but values are not written to (like variables), but defined.


You are right that values are not written. Still, I am very happy with this name due to the llvm::Value correspondance.

> I kind of liked the distinction of implicit/explicit (i.e. whether it comes from an explicit LoadInst/StoreInst or it is a result of how we model scalars), but it might also be orthogonal to the array kind.


Right.


================
Comment at: include/polly/ScopInfo.h:120
@@ +119,3 @@
+  enum KIND {
+    /// * KIND_ARRAY: Models a multi-dimensional array
+    ///
----------------
Meinersbur wrote:
> array of arbitrary dimensionality.
> 
> multi-dimensional could be understood as at least 2 dimensions.
Now:

"Models a one or multi-dimensional array"

================
Comment at: include/polly/ScopInfo.h:605
@@ -581,3 +604,3 @@
                ArrayRef<const SCEV *> Subscripts, ArrayRef<const SCEV *> Sizes,
-               Value *AccessValue, AccessOrigin Origin, StringRef BaseName);
+               Value *AccessValue, ARRAYKIND ArrayKind, StringRef BaseName);
   ~MemoryAccess();
----------------
Meinersbur wrote:
> ScopArrayInfo::KIND, ARRAYKIND, KIND_ARRAY is confusing.
> 
> I don't think ScopArrayInfo::KIND is too long to require a typedef, but if, it should have the same name as the declaration in ScopArrayInfo. The individual values (e.g. ScopArrayInfo::KIND_ARRAY) still need to be fully qualified.
> 
>  [[ http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly | LLVM's style guide ]] says the enum type names should not be all caps, and uses "Kind" as suffix instead of prefix.
> ScopArrayInfo::KIND, ARRAYKIND, KIND_ARRAY is confusing.

We are now left with ScopArrayInfo::MemoryKind and
ScopArrayInfo::MK_Array.

>I don't think ScopArrayInfo::KIND is too long to require a 
> typedef, but if, it should have the same name as the
> declaration in ScopArrayInfo. The individual values (e.g. 
> ScopArrayInfo::KIND_ARRAY) still need to be fully 
> qualified.

I dropped the typedef.

> LLVM's style guide says the enum type names should 
> not be all caps, and uses "Kind" as suffix instead of 
> prefix.

I now follow the llvm style guide:

"Enumerators (e.g. enum { Foo, Bar }) and public member variables should start with an upper-case letter, just like types"

"enumerators should have a prefix corresponding to the enum declaration name. For example, enum ValueKind { ... }; may contain enumerators like VK_Argument, VK_BasicBlock, etc"

================
Comment at: include/polly/ScopInfo.h:700
@@ -676,3 +699,3 @@
   /// @brief Whether this is an access of an explicit load or store in the IR.
-  bool isExplicit() const { return Origin == EXPLICIT; }
+  bool isKindArray() const { return ArrayKind == ScopArrayInfo::KIND_ARRAY; }
 
----------------
Meinersbur wrote:
> The function name sounds gramatically wrong.
I now moved to hasKindArray(). Does this sound better?

I could also use isArrayKind(), but I would like the different Kind* functions to be close by alphabetically such that they can be easy found when autocompleting.




http://reviews.llvm.org/D15307