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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:62
 
+enum class RegKind {Scalar, NeonVector, SVEVector };
+
----------------
rengolin wrote:
> 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"?
Fair point, it should probably distinguish Data and Predicate in the name. I'll change this.
(also, since this is not yet used until the next patch in this series, I will move this change to that patch)


================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:813
+
+  bool isNeonVectorReg() const {
+    return Kind == k_Register && Reg.Kind == RegKind::NeonVector;
----------------
rengolin wrote:
> Is there a situation (in the future) that we might want?
> 
>     bool isVector() const { return Kind == k_Register && !Reg.isScalar; }
At the moment I haven't found such a situation yet (even when considering other SVE assembler/disassembler patches) and I would rather not add code that isn't used anywhere.
So I suggest adding that only when needed.


https://reviews.llvm.org/D39088