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

[Jonas Paulsson via Phabricator via llvm-commits]

jonpa 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,
----------------
uweigand wrote:
> 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.
Ah, I guess I was looking at the GNU source code (opcodes_s390-opc.c) and used the instructions given as examples there as models for my patch...

Changed to BDAddr64Displ12.



================
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),
----------------
uweigand wrote:
> 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.)
Ah, I see. Changed to VR128 for the vector reg operands.

I tried removing the vector regs from AnyReg register class, but that did not produce an error:


```
  .insn rr,0x1800,%r2,%r0  =>   0:   18 20                   lr      %r2,%r0
  .insn rr,0x1800,%v2,%r0  =>   0:   18 20                   lr      %r2,%r0

```

I also checked with GAS, and the behavior was the same (registers just parsed for the numbers, and the preceeding letter (r/v/f...) seems ignored). Since this matches GAS, I did not take pursue this further...(?)





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

https://reviews.llvm.org/D88357