[PATCH] D95804: [ARM] MVE vector lane interleaving

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer accepted this revision.
SjoerdMeijer added a comment.
This revision is now accepted and ready to land.

Looks like a nice change to me.



================
Comment at: llvm/lib/Target/ARM/MVELaneInterleavingPass.cpp:127
+  // But expensive extends/truncs are always good to remove.
+  for (auto *E : Exts)
+    if (!isa<LoadInst>(E->getOperand(0))) {
----------------
Nit: not sure what the coding standard says about this case, but perhaps some curly brackets are required for this for-loop and the one below.


================
Comment at: llvm/lib/Target/ARM/MVELaneInterleavingPass.cpp:199
+    case Instruction::Select:
+      if (Ops.count(I))
+        continue;
----------------
Nit: if we are only using `Ops` to avoid revisiting the same instruction, perhaps `Seen` is a better description.... (see next comment)


================
Comment at: llvm/lib/Target/ARM/MVELaneInterleavingPass.cpp:225
+    dbgs() << "  Ops:";
+    for (auto *I : Ops)
+      dbgs() << "  " << *I << "\n";
----------------
....because at this point I was curious how else it was used, but don't think it is.


================
Comment at: llvm/lib/Target/ARM/MVELaneInterleavingPass.cpp:1
+//===- MVELaneInterleaving.cpp - Inverleave for MVE instructions ----------===//
+//
----------------
dmgreen wrote:
> SjoerdMeijer wrote:
> > Before I do a 2nd and more in depth review, some name bike shedding: Lane Interleaving.
> > I am wondering if that is the most accurate description. This pass is about more efficient vector (element) extensions, which is achieved by some reshuffling. But lane interleaving is something that is used under the hood so we can use the bottom/top moves, thus it's not about lane interleaving per se or more efficient codegen for lane interleaving.  So, would MVEVectorExtends or something like that be a better description?
> Lane interleaving sounds like the right name to me. The idea of the pass is to interleave the lanes so that we can use t/b instructions, as the extended vector uses a different lane ordering to the original.
> The vector extends are just one way of doing that. There will hopefully be follow up patches for things like constant arrays and interleaving existing shuffles. I was keeping this patch simple though.
Okidoki, sounds good.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95804/new/

https://reviews.llvm.org/D95804