[PATCH] D63567: [ARM] Mve vector shuffles

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:6888
+
+static bool isLegalMveShuffleOp(unsigned PFEntry) {
+  unsigned OpNum = (PFEntry >> 26) & 0x0F;
----------------
a proper nit: perhaps Mve -> MVE for consistency


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:7191
+      else if (isLegalMveShuffleOp(PFEntry)) {
+        unsigned LHSID = (PFEntry >> 13) & ((1 << 13)-1);
+        unsigned RHSID = (PFEntry >>  0) & ((1 << 13)-1);
----------------
There's a lot of prior art here of magic constants and shifts etc., but this doesn't mean much to me.
It probably makes sense when you're familiar with querying the shuffle table. 
Not sure if it is worth a comment, but from a quick look at code here, that might not be even necessary.


================
Comment at: llvm/test/CodeGen/Thumb2/mve-shuffle.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -O3 -mtriple=thumbv8.1m.main-arm-none-eabi -mattr=+mve.fp %s -o - | FileCheck %s
+
----------------
Do we need tests when we don't have HasMVEInt? Or is that not really useful (or done elsewhere already)?


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

https://reviews.llvm.org/D63567