[PATCH] D45427: [AArch64][AsmParser] Unify code for parsing Neon/SVE vectors.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

nice cleanup, thanks. Just a few minor comments.



================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:1853
-      .Case(".1q", true)
-      // Accept the width neutral ones, too, for verbose syntax. If those
-      // aren't used in the right places, the token operand won't match so
----------------
Please keep those comments.


================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:1859
 
-static bool isValidVectorKind(StringRef Name) {
-  return StringSwitch<bool>(Name.lower())
-      .Case(".8b", true)
-      .Case(".16b", true)
-      .Case(".4h", true)
-      .Case(".8h", true)
-      .Case(".2s", true)
-      .Case(".4s", true)
-      .Case(".1d", true)
-      .Case(".2d", true)
-      .Case(".1q", true)
-      // Accept the width neutral ones, too, for verbose syntax. If those
-      // aren't used in the right places, the token operand won't match so
-      // all will work out.
-      .Case(".b", true)
-      .Case(".h", true)
-      .Case(".s", true)
-      .Case(".d", true)
-      // Needed for fp16 scalar pairwise reductions
-      .Case(".2h", true)
-      // another special case for the ARMv8.2a dot product operand
-      .Case(".4b", true)
-      .Default(false);
+static Optional<std::pair<int, int> > parseVectorKind(StringRef Suffix,
+                                                      RegKind VectorKind) {
----------------
Can you add a brief comment to document what the values in the returned pair mean?


================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:1866
+    Res = StringSwitch<std::pair<int, int> >(Suffix.lower())
+              .Case("", { 0, 0 })
+              .Case(".1d", { 1, 64 })
----------------
Why are we matching "" for neon here? Looks like we did not match that before (on a first look)?


================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:2871
+  // Wrapper around parse function
+  auto ParseVector = [&](int &Reg, StringRef &Kind) {
+    auto StartLoc = getLoc();
----------------
Would it be enough to capture this here? Or just pass the location explicitly?


================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:2877
+      const auto &Res = parseVectorKind(Kind, RegKind::NeonVector);
+      assert(Res && "Expected a valid vector kind");
+      return true;
----------------
This will trigger an unused variable warning when assertions are enabled I think. Maybe just move parseVectorKind in the assert?


https://reviews.llvm.org/D45427