[PATCH] D39088: [AArch64][SVE] Asm: Replace 'IsVector' by 'RegKind' in AArch64AsmParser (NFC)

[Renato Golin via Phabricator via llvm-commits]

rengolin added a comment.

So, overall, this is a sensible change. We must be able to separate Neon vectors from SVE vectors. I'm not sure how the predicates will fare here and why they wouldn't be SVE vectors (surely, they're scalable, too).

I would prefer to keep `isVector` and add `isScalable`, and move the distinction between Neon and SVE to helper functions. But I'm not holding this opinion firmly, as I haven't seen the rest of the code.



================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:62
 
+enum class RegKind {Scalar, NeonVector, SVEVector };
+
----------------
sdesmalen wrote:
> javed.absar wrote:
> > fhahn wrote:
> > > Would it make sense to use something more generic for the names here, like `ScalableVector`?
> > It seems to be it would be more maintainable to split it 
> > - RegKind { Scalar, Vector };
> >  and then 
> > enum class VectorKind { NeonVector, SVEVector};
> > 
> > This might simplify the checking logic further down.
> @fhahn A subsequent patch will also add a SVE Predicate Vector.
> 
> @javed.absar I had a look at splitting it into RegKind { Scalar, Vector }; and subsequently into VectorKind {NeonVector, SVEVector}, but I don't easily see how this makes the checking logic any simpler. Did you have a specific check that you think would be simplified by this?
Why won't SVE predicate not be an "SVEVector"?


================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:2771
+      FirstReg, Count, NumElements, ElementKind, S, getLoc(),
+      getContext()));
 
----------------
fhahn wrote:
> unrelated change?
just checked, this is not violating 80-columns. :)


================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:813
+
+  bool isNeonVectorReg() const {
+    return Kind == k_Register && Reg.Kind == RegKind::NeonVector;
----------------
Is there a situation (in the future) that we might want?

    bool isVector() const { return Kind == k_Register && !Reg.isScalar; }


================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:1898
+  case RegKind::NeonVector:
+    RegNum = matchNeonVectorRegName(Name);
+    break;
----------------
function naming consistency.


================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:1912
+    if (Kind == Entry->getValue().first)
+      return Entry->getValue().second;
   }
----------------
not that it matters much but the return change here is pointless. :)


https://reviews.llvm.org/D39088