[PATCH] D23646: Generalize strided store pattern in interleave access pass

[Renato Golin via llvm-commits]

rengolin added inline comments.


================
Comment at: lib/CodeGen/InterleavedAccessPass.cpp:193
+      for (j = 0; j < NumSubElts-1; j++) {
+        unsigned ij = j*Factor + i;
+        if (Mask[ij] >= 0 && Mask[ij + Factor] >= 0 &&
----------------
Nit, `ij` is hard to follow. Try `lane` or something more expressive. (this is not a matrix :)


================
Comment at: lib/CodeGen/InterleavedAccessPass.cpp:204
+        // corresponding distance.
+        if (PreviousMask > 0 && Mask[ij] < 0 && Mask[ij + Factor] >= 0 &&
+            static_cast<unsigned>(PreviousMask) + (j - PreviousPos) + 1  !=
----------------
This is really confusing. Can you factor the comparison elements out with expressive names, so the if becomes a comparison of obvious terms?


================
Comment at: lib/CodeGen/InterleavedAccessPass.cpp:208
+            break;
+        if (Mask[ij] >= 0 && Mask[ij + Factor] < 0) {
+          PreviousMask = Mask[ij];
----------------
PreviousMask is always used in conjunction with PreviousPos, so you don't need the mask to be signed and you can compare the pos in the block above and get rid of the static casts. Or you could have an additional boolean flag and make them both unsigned.


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:7243
+        Op0, Op1, getSequentialMask(Builder, Mask[i], NumSubElts)));
+    else {
+      unsigned StartMask = 0;
----------------
Nit. use brackets here:

    if (...) {
      ...
    } else {


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:7247
+        if (Mask[j*Factor + i] >= 0) {
+          StartMask = Mask[j*Factor + i] - j;
+          break;
----------------
I don't get the `- j` here.


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:13126
+      }
+      // Correctness notes in AArch64ISelLowering apply here as well.
+      Ops.push_back(Builder.CreateShuffleVector(
----------------
Better to duplicate the comment, I think. These back-ends evolve at different paces.


================
Comment at: test/CodeGen/ARM/arm-interleaved-accesses.ll:321
+; NEON-LABEL: store_general_mask_factor4:
+; NEON: vst4.32 {d18, d19, d20, d21}, [r0]
+; NONEON-LABEL: store_general_mask_factor4:
----------------
is this really guaranteed to reproduce? they don't seem connected to the pcs directly...


https://reviews.llvm.org/D23646