[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 };
+
----------------
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?


================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:811
+  bool isReg() const override {
+    return Kind == k_Register && Reg.Kind == RegKind::Scalar;
+  }
----------------
fhahn wrote:
> Personally I think having predicate functions like `Reg.isScalar()`, `Reg.isNeonVector()` would be slightly more compact, but I think either way is fine.
I think that is basically what we're implementing here, but hen for 'isReg()'. See for example 'isVectorReg()', which is the opposite of 'isScalar()'. I see now that isVectorReg() is only checking for a NeonVector, which seems inconsistent with its name. I'll see if I can fix that.


================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:1889
 
+unsigned AArch64AsmParser::matchReqRegisterNameAlias(StringRef Name,
+                                                     RegKind Kind) {
----------------
fhahn wrote:
> What is the reason for moving this code out to a separate function? Is it to not change the interface of `matchRegisterNameAlias`, which still takes `bool isVector` as an argument?
We initially moved it into a separate function to reuse it for 'matchSVEVectorNameAlias()' (as added in patch 3/5)... But, looking at it again I see it makes more sense to add to the same function and use RegKind for each (which eliminates the purpose of moving this into a separate function). Please check if you're happy with the next iteration of this patch.


================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:1905
 unsigned AArch64AsmParser::matchRegisterNameAlias(StringRef Name,
                                                   bool isVector) {
   unsigned RegNum = isVector ? matchVectorRegName(Name)
----------------
fhahn wrote:
> I think keeping the boolean `isVector` argument has potential to confuse people. Any reason for not being explicit here and pass RegKind here? Because there are 2 vector register types now, `isVector` seems ambiguous.
should be addressed by the next iteration of this patch (see previous comment)


https://reviews.llvm.org/D39088