[PATCH] [AArch64] Match interleaved memory accesses into ldN/stN instructions.

[James Molloy]

Hi Hao,

I have some comments below. I think it would have been nicer if we'd managed to get this into CodeGen, but I do see the difficulties there. It's hard enough to match at the IR level!

My larger concern is how we're going to do this for ARM32 too without duplicating the entire pass... If there were somewhere we could put a templated version of this pass that ARM could use too, that would be great. I'm not a fan of copy-paste on this scale.

Cheers,

James


================
Comment at: lib/Target/AArch64/AArch64InterleavedAccess.cpp:60
@@ +59,3 @@
+/// This enum is just used to hold the minimum and maximum interleave factor.
+enum {
+  MIN_FACTOR = 2, /// Minimum interleave factor
----------------
This would probably be easier simply as two const ints. Enum is overkill.

================
Comment at: lib/Target/AArch64/AArch64InterleavedAccess.cpp:161
@@ +160,3 @@
+    ShuffleVectorInst *SVI, SmallVector<Instruction *, 16> &DeadInsts) {
+  LoadInst *LI = dyn_cast<LoadInst>(SVI->getOperand(0));
+  if (!LI || !LI->isSimple() || !isa<UndefValue>(SVI->getOperand(1)))
----------------
I would expect here you to check whether SVI is in DeadInsts, as we may have identified it earlier. If you don't, I'd expect we'd create multiple LDN instructions unnecessarily!

================
Comment at: lib/Target/AArch64/AArch64InterleavedAccess.cpp:172
@@ +171,3 @@
+
+  // Check if the mask is strided and get the start index.
+  unsigned Factor, Index;
----------------
The order of these checks doesn't seem the most efficient to me. You have a load that has at least one ShuffleVector using it. You're checking the ShuffleVector is a valid mask first, then checking all other Load users.

I'd expect the code would be simpler if:
  1. Check the illegal vector type size.
  2. Check the load only had ShuffleVector users.
  3. Check each ShuffleVector is a strided index.

I guess what I think is strange is that this function takes a ShuffleVector as its argument, rather than a LoadInst.

================
Comment at: lib/Target/AArch64/AArch64InterleavedAccess.cpp:236
@@ +235,3 @@
+    // Avoid analyzing it twice.
+    if (SV != SVI)
+      SV->eraseFromParent();
----------------
This is weird - surely you should just put it into DeadInsts?

================
Comment at: lib/Target/AArch64/AArch64InterleavedAccess.cpp:250
@@ +249,3 @@
+/// I.e. <0, NumSubElts, ... , NumSubElts*(Factor - 1), 1, NumSubElts + 1, ...>
+static bool IsInterleavedMaskOfGivenFactor(ArrayRef<int> Mask,
+                                           unsigned Factor) {
----------------
Please move this static function up to where all the other static functions are.

I got confused initially how this function was different to isStridedMask above. The difference is that this function checks for a mask that RE-interleaves, whereas isStridedMask checks for a mask that DE-interleaves. Please make that more clear in the function name / comments.

================
Comment at: lib/Target/AArch64/AArch64TargetTransformInfo.cpp:416
@@ +415,3 @@
+  if (Factor > 1 && Factor < 5 && isTypeLegal(VecTy))
+    return Factor;
+
----------------
Why does a ld2 have a different cost to a ld3 here?

http://reviews.llvm.org/D10335

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/