[PATCH] D62669: [ARM] Set up infrastructure for MVE vector instructions.

[Oliver Stannard (Linaro) via Phabricator via llvm-commits]

ostannard added inline comments.


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:504
   }
+  bool hasMVEInt() const {
+    return getSTI().getFeatureBits()[ARM::HasMVEIntegerOps];
----------------
mve.fp implies mve.int, so one of these functions is redundant.


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:2114
+      return false;
+    ARMCC::CondCodes CC = (ARMCC::CondCodes)getCondCode();
+    return CC == ARMCC::EQ || CC == ARMCC::NE;
----------------
getCondCode already returns an ARMCC::CondCodes, so these casts aren't needed.


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:6317
+        (Operand->isReg() &&
+         (ARMMCRegisterClasses[ARM::MQPRRegClassID].contains(
+              Operand->getReg()) ||
----------------
MQPR is a subset of QPR, so there's no point in checking both.


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:6457
       Mask >>= 1;
-      if (ITMask[i - 1] == 't')
+      if (ITMask[i - 1] == (Mnemonic.startswith("vp") ? 'e' : 't'))
         Mask |= 8;
----------------
Why is this inverted compared to IT? Is the format of these masks written down anywhere?


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:6907
+  if (inVPTBlock() && !instIsBreakpoint(Inst)) {
+    unsigned Bit = 0;
+    if (VPTState.CurPosition != 0)
----------------
Could the bit-twiddling on the VPS state mask be split into a helper function, like currentITCond?


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:6925
+  }
+  else if (hasMVE() && isVectorPredicable(MCID) &&
+           Inst.getOperand(findFirstVectorPredOperandIdx(MCID)).getImm() !=
----------------
Why do we need to check hasMVE here?


================
Comment at: llvm/lib/Target/ARM/MCTargetDesc/ARMMCCodeEmitter.cpp:534
     unsigned RegNo = CTX.getRegisterInfo()->getEncodingValue(Reg);
+    if (STI.getFeatureBits()[ARM::HasMVEIntegerOps] ||
+        STI.getFeatureBits()[ARM::HasMVEFloatOps])
----------------
I think this needs a comment explaining what is different about MVE.


================
Comment at: llvm/test/MC/ARM/mve-vpt.s:9
+# CHECK-NOFP: vabav.s8  r0, q1, q3 @ encoding: [0x82,0xee,0x07,0x0f]
+vabav.s8 r0, q1, q3
+
----------------
Are these instructions testing anything related to VPT blocks? If not, they should be moved to a separate file (and we should check the actual error which is emitted in the non-FP case).


================
Comment at: llvm/test/MC/ARM/mve-vpt.s:66
+# CHECK: vptete.f16 ne, q0, q1 @ encoding: [0x71,0xfe,0x82,0xef]
+# CHECK-NOFP-NOT: vptete.f16 ne, q0, q1 @ encoding: [0x71,0xfe,0x82,0xef]
+vptete.f16 ne, q0, q1
----------------
We should also check the errors which are reported for invalid instructions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62669