This is an archive of the discontinued LLVM Phabricator instance.

[SystemZ] Use named MI sub-operands
ClosedPublic

Authored by iii on Jul 13 2023, 6:05 AM.

Details

Summary

Prepare for removing the MemOpsEmitted workaround for symbolic
displacements by letting TableGen know about the offsets of the
displacement sub-operands within the instruction.

There are alternative ways to do this that were tried and rejected:

  • Creating encoders and decoders for each possible displacement offset. This is too repetitive.
  • Use VarLenCodeEmitter [1]. The resulting diff is quite large.

Instead, use the named sub-operand support introduced by commit
a538d1f13a13 ("[TableGen][CodeEmitterGen] Allow local names for
sub-operands in a operand list.").

Describe instruction encodings in terms of sub-operands instead of
operands (e.g. B, D, L vs BDL) - this also better matches the pictures
from the Principles of Operation. Decompose operands into sub-operands
using the new (bdaddr12only $B1, $D1):$BD1 syntax. Replace the
encoders and the decoders of the operands with these of the
sub-operands.

Since DecodeADDR64BitRegisterClass() is now used for bases and indices,
change it to return NoRegister when decoding 0. This also changes the
disassembly of some instructions, e.g., br %r0 becomes br 0. Since this
better captures the instruction semantics, namely, that the value of
%r0 is not used, keep this change and update the tests.

[1] https://m680x0.github.io/blog/2022/02/varlen-encoder.html

Diff Detail

Event Timeline

iii created this revision.Jul 13 2023, 6:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2023, 6:05 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
iii requested review of this revision.Jul 13 2023, 6:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2023, 6:06 AM
uweigand accepted this revision.Jul 13 2023, 9:43 AM

This looks really nice, thanks for that improvement! A couple of trivial nits inline, otherwise LGTM.

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

Minor nit: can you move this next to the InstRSY definitions?

2857–2858

Looks like you accidentally removed the space after comma here.

5167

I see, the original $XBD was actually a bug. Good catch!

This revision is now accepted and ready to land.Jul 13 2023, 9:43 AM
iii updated this revision to Diff 540189.Jul 13 2023, 2:54 PM
iii marked an inline comment as done.
  • Rebase (the dependency CL was merged).
  • New prep patch: ADDR64/GR64 cleanup.
  • Move InstRSEa.
  • Improve whitespace.
iii marked 2 inline comments as done.Jul 13 2023, 2:55 PM
iii updated this revision to Diff 540405.Jul 14 2023, 7:06 AM
  • Drop the ADDR64/GR64 patch.
  • Adjust the tests to accept 0 instead of %r0 instead.
  • There already are a lot of assembler tests that check that %r0 is accepted for ADDR64.
iii retitled this revision from [SystemZ][NFC] Use named sub-operands in MC to [SystemZ] Use named MI sub-operands.Jul 14 2023, 7:52 AM
iii edited the summary of this revision. (Show Details)
uweigand accepted this revision.Jul 14 2023, 8:03 AM

Yes, I agree that this is the best approach for the ADD64 issue. This version LGTM, thanks!

This revision was automatically updated to reflect the committed changes.