This is an archive of the discontinued LLVM Phabricator instance.

[SystemZ] Add support for .insn directives for vector instructions
ClosedPublic

Authored by jonpa on Sep 26 2020, 6:40 AM.

Details

Summary

Support VRI, VRR, VRS, VRV, VRX, VSI instruction formats with the .insn directive.

Diff Detail

Event Timeline

jonpa created this revision.Sep 26 2020, 6:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 26 2020, 6:40 AM
jonpa requested review of this revision.Sep 26 2020, 6:40 AM
uweigand added inline comments.Oct 2 2020, 9:56 AM
llvm/lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp
643

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.

llvm/lib/Target/SystemZ/SystemZInstrFormats.td
1767

I'd prefer to just name this DirectiveInsnVRI (without the "e") like all the others.

llvm/lib/Target/SystemZ/SystemZInstrInfo.td
2246

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.)

2256

Given the comment in SystemZAsmParser.cpp, this should then be bdaddr12only instead of shift12only.

jonpa updated this revision to Diff 296143.Oct 5 2020, 3:50 AM
jonpa marked 4 inline comments as done.

Updated per review.

llvm/lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp
643

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.

llvm/lib/Target/SystemZ/SystemZInstrInfo.td
2246

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...(?)

uweigand accepted this revision.Oct 5 2020, 4:44 AM

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

llvm/lib/Target/SystemZ/SystemZInstrInfo.td
2246

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) ...

This revision is now accepted and ready to land.Oct 5 2020, 4:44 AM