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

Mon Oct 5 04:44:23 PDT 2020

uweigand accepted this revision.
uweigand added a comment.
This revision is now accepted and ready to land.

See the inline question about vector registers.  This does not affect the current patch however, so this should be good to go in.  LGTM.

================
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),
----------------
jonpa wrote:
> 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...(?)
> 
> 
> 
Can you also check with a register number >= 16?   For < 16, I guess there's no real difference between %vN and %fN (which is allowed for AnyReg) ...

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

https://reviews.llvm.org/D88357