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

[Alina Sbirlea via llvm-commits]

asbirlea 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 &&
----------------
rengolin wrote:
> Nit, `ij` is hard to follow. Try `lane` or something more expressive. (this is not a matrix :)
Renamed to Lane. I hope changing the NumSubElts to LaneLen makes more sense too. 
I'm inclined to change it in the lowering files as well for consistency.


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:7247
+        if (Mask[j*Factor + i] >= 0) {
+          StartMask = Mask[j*Factor + i] - j;
+          break;
----------------
rengolin wrote:
> I don't get the `- j` here.
Assuming the mask starts with a few undefs, this computes what the start of the mask would be based on the first non-undef value. 
The computation is done first in the pass to make sure the start is a positive value (hence the correctness comment below on "StartMask cannot be negative")


================
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:
----------------
rengolin wrote:
> is this really guaranteed to reproduce? they don't seem connected to the pcs directly...
I'm not sure about this TBH, and not sure how to verify it. 
Should I replace it by a simple vst4.32 check?


https://reviews.llvm.org/D23646