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

[Florian Hahn via Phabricator via llvm-commits]

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

LGTM



================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:1866
+    Res = StringSwitch<std::pair<int, int> >(Suffix.lower())
+              .Case("", { 0, 0 })
+              .Case(".1d", { 1, 64 })
----------------
sdesmalen wrote:
> fhahn wrote:
> > Why are we matching "" for neon here? Looks like we did not match that before (on a first look)?
> That is because this function can also be called when the Suffix is an empty string, where previously 'isValidVectorKind()' was only called when Kind was non-empty. See for instance 'tryParseNeonVectorRegister()' which unconditionally calls 'parseVectorKind()' in order to pass ElementWidth to the CreateVectorReg() call.
Got it, thanks.


================
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;
----------------
sdesmalen wrote:
> fhahn wrote:
> > This will trigger an unused variable warning when assertions are enabled I think. Maybe just move parseVectorKind in the assert?
> If I put the 'parseVectorKind' itself in an assert, it might not get executed (which is needed for 'Kind' to be set, which is passed by reference). I'll rewrite it into a condition with the  'else' being a llvm_unreachable.
Got it, thanks.


================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:1876
+              .Case(".1q", { 1, 128 })
+          // '.2h' needed for fp16 scalar pairwise reductions
+              .Case(".2h", { 2, 16 })
----------------
The indents here seem weird. Can you run clang-format-diff before committing?


https://reviews.llvm.org/D45427