[PATCH] D88357: [SystemZ] Add support for .insn directives for vector instructions

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand added inline comments.


================
Comment at: llvm/lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp:643
+  { "vrs", SystemZ::InsnVRS, 5,
+    { MCK_U48Imm, MCK_AnyReg, MCK_AnyReg, MCK_BDAddr32Disp12, MCK_U4Imm } },
+  { "vrv", SystemZ::InsnVRV, 4,
----------------
I'm wondering why you're using BDAddr32 here; while there are shifts using the VRS format, there are also other instruction using the VRS format where the base register is used as 64-bit.   While we don't know which instruction this is being using for, I think it would be better (and simpler) to use BDAddr64Disp12 here.


================
Comment at: llvm/lib/Target/SystemZ/SystemZInstrFormats.td:1767
 
+class DirectiveInsnVRIe<dag outs, dag ins, string asmstr, list<dag> pattern>
+  : InstVRIe<0, outs, ins, asmstr, pattern> {
----------------
I'd prefer to just name this DirectiveInsnVRI (without the "e") like all the others.


================
Comment at: llvm/lib/Target/SystemZ/SystemZInstrInfo.td:2246
+  def InsnVRIe : DirectiveInsnVRIe<(outs),
+                                   (ins imm64zx48:$enc, AnyReg:$V1, AnyReg:$V2,
+                                    imm32zx12:$I3, imm32zx4:$M4, imm32zx4:$M5),
----------------
I think we should not use AnyReg for vector registers here, but rather VR128.  Note that these are not interchangable as vector regs need 5 bits while all other regs only need 4, so it matters whether the format has a slot for a VR vs. any other reg.

In fact, I think AnyReg should probably not even accept vector registers; passing a VR to any of the *other* insn formats that do not expect a VR will lose one bit.  (You might want to confirm with how this is handled by GAS as well.)


================
Comment at: llvm/lib/Target/SystemZ/SystemZInstrInfo.td:2256
+                                 (ins imm64zx48:$enc, AnyReg:$R1, AnyReg:$V3,
+                                  shift12only:$BD2, imm32zx4:$M4),
+                                 ".insn vrs,$enc,$BD2,$M4", []>;
----------------
Given the comment in SystemZAsmParser.cpp, this should then be bdaddr12only instead of shift12only.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88357/new/

https://reviews.llvm.org/D88357