[PATCH] D36960: [X86][LLVM]Expanding Supports lowerInterleavedLoad() in X86InterleavedAccess (VF{8|16|32} stride 3).

[Zvi Rackover via Phabricator via llvm-commits]

zvi added inline comments.


================
Comment at: lib/Target/X86/X86InterleavedAccess.cpp:368
+
+  for (Index = Mask.size() - 1; Index >= 0; Index--) {
+    if (GroupNum != Mask[Index] % Stride) {
----------------
m_zuckerman wrote:
> zvi wrote:
> > I might be missing something, but is group number 0 defined to be the group starting at the end of Mask?
> Group number zero ( According to the modulo operation) will always lie at the first element of the shuffle since we begin with zero.For example, think about a vector length 8 with stride 3. The shuffle sequence will look like that [0,3,6,1,4,7,2,5] so we don't know the last group but we can know what is the first element since we begin the shuffle from zero. 
I see in the examples above that the first group consists of elements that are the last indices of Mask, and the search starts from the last index.


================
Comment at: lib/Target/X86/X86InterleavedAccess.cpp:346
+                                SmallVectorImpl<uint32_t> &Mask) {
+  int LaneCount = std::max(VectorSize / 128, 1);
+  for (int Lane = 0; Lane < LaneCount; Lane++)
----------------
On second thought no need for the max() with  assert(vectorSize >= 128)


================
Comment at: lib/Target/X86/X86InterleavedAccess.cpp:348
+  for (int Lane = 0; Lane < LaneCount; Lane++)
+    for (int i = 0, LaneSize = VF / LaneCount; i != LaneSize; ++i) {
+      Mask.push_back((i * Stride) % (LaneSize) + (LaneSize) * Lane);
----------------
these brackets can be dropped


================
Comment at: lib/Target/X86/X86InterleavedAccess.cpp:349
+    for (int i = 0, LaneSize = VF / LaneCount; i != LaneSize; ++i) {
+      Mask.push_back((i * Stride) % (LaneSize) + (LaneSize) * Lane);
+    }
----------------
Extra brackets


================
Comment at: lib/Target/X86/X86InterleavedAccess.cpp:353
+
+//  getGroupElemCount returns the size of group.
+//  There are Stride different groups. Each group can be calculated according to
----------------
Please consider something like this:

```
// getGroupElemCount - returns the size in elements of group index 'Group' in mask 'Mask'.
// A mask contains exactly 'Stride' groups, where each group is a monotonically increasing sequence with stride 'Stride'.
// In these examples the interested group is marked between '[...]'
//  getGroupElemCount(3, 0, {0|3|6|1|4|7|[2|5]}) -> 2
//  getGroupElemCount(3, 1, {0|3|6|[1|4|7]|2|5}) -> 3
```


================
Comment at: lib/Target/X86/X86InterleavedAccess.cpp:363
+  int Index = 0, Count = 0, GroupElemCount = 0;
+  int GroupNum = Mask[Mask.size() - 1] % Stride;
+
----------------
Mask.back() % Stride


https://reviews.llvm.org/D36960