[PATCH] D39089: [AArch64][SVE] Asm: Add SVE (Z) Register definitions and parsing support

[Renato Golin via Phabricator via llvm-commits]

rengolin added inline comments.


================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:844
+    return (Kind == k_SVERegister ||
+            (Kind == k_Register && Reg.Kind == RegKind::SVEDataVector)) &&
+           AArch64MCRegisterClasses[Class].contains(getReg());
----------------
sdesmalen wrote:
> rengolin wrote:
> > rengolin wrote:
> > > To @javed.absar comment in D39088, this may be simplified by having all done at the RegKind level, instead of having multiple types of registers on top of multiple types of SVE vectors.
> > I'm confused now. Why do we need both ways of identifying an SVE register?
> > 
> > I can't see anything in this patch that needs `k_SVEDataRegister`. All uses of it could very well be together with `k_Register` and have an additional check for `RegKind::SVEDataVector`.
> Okay, I understand the confusion, and its mostly caused by this patch not containing the complete context of how future patches extend SVEDataRegister. I'll try and provide some more context in my future patches, but let me describe this particular instance here:
> 
> k_SVEDataRegister will be extended with more details such as 'ElementWidth' and details about sign/shift extension. In contrast, k_Register and Reg.Kind == RegKind::SVEDataVector describe a vector register as a 'bare' register. At this stage, it is functionally equivalent to using k_Register, so this is more a non-functional change for now to prepare for future patches.
Right, now I get it. `SVEDataRegOp` has an `RegOp` inside it, which composes the structure inside the union.

This is a little confusing, and I'd have just extended `RegOp` instead. This is an AArch64-only structure/union, and AArch64 registers now are a little more complicated, and I can't see why changing `RegOp` would break anything, if you just extend it. Something like:

    struct RegOp {
      unsigned RegNum;
      RegKind Kind;
      int ElementWidth;
    };

then...

    Width = isSVEVectorReg(Reg) ? Reg.RegOp.ElementWidth : 1;

current uses of RegOp will not access the new elements.


https://reviews.llvm.org/D39089