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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen 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());
----------------
rengolin wrote:
> sdesmalen wrote:
> > rengolin wrote:
> > > 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.
> > ElementWidth and any Sign/Shift extend are not necessarily things that apply to any RegOp and is specific to the kind of operand you're parsing, so fiddling both scalar, vector and sign/shift-extend specific information into a generic 'RegOp' even when it does not apply to the operand seems conceptually a bit odd to me. I wonder if it would be better to have this discussion when adding support for element-width/shift-extend, as we then have a better picture what things look like?
> Well, you're already adding `ElementWidth` now, signs and shifts will only complement, not change the idea.
> 
> You have added `Kind` to `RegOp`, which defines what it is, not only if it's a vector or not. This already makes `RegOp` a bucket for multiple types of registers. But then you create an `SVERegOp` which is another bucket, for a slightly different type of vector. Now we have three vector types and two operand types.
> 
> We either have one, with a type and extra fields that only make sense to specific types, or we have three operand types, one for each type of register we have.
Fair enough, I tried this out and found that it would also simplify the code further down the line. Please see the updated patch for the change.


https://reviews.llvm.org/D39089