[PATCH] D113341: [SystemZ] Support symbolic displacements.

[Jonas Paulsson via Phabricator via llvm-commits]

jonpa updated this revision to Diff 387256.
jonpa added a comment.

> 1. We know that the encoding callbacks are called by TableGen-generated code in operand order.   So we could simply track at which memory operands we are by counting them (i.e. add a member `MemOpsEmitted` or so to the `SystemZMCCodeEmitter` class, reset it to zero at the beginning of each instruction in `encodeInstruction` and increment it whenever emitting any of the BD* type operands).   Then we could simply assume that the first memory operand has a byte offset of 2, and the second a byte offset of 4.   This wouldn't even require any `TSFlags` bit.

I tried this and it was simple enough - with just an increment in a single place in getDispOpValue(). However, encodeInstruction() is an overriden const function, so clearing MemOpsEmitted wass not permitted. Then it occured to me that if we really can make this broad assumption, we could just check the size of the Fixups vector and that way differentiate between the first and second fixup.

> As to the bit offset, I believe this counts from the LSB, and so it would actually be zero for all of our fields.

I tried clearing the bit offsets, but that broke some tests. It looks like the commenting algorithm is using these:

  With bit offset: la	%r1, b-a(%r1)                   # encoding: [0x41,0x10,0b0001AAAA,A]
  Without        : la	%r1, b-a(%r1)                   # encoding: [0x41,0x10,0x10'A',0bAAAA0000]    ### WRONG

So in this context it seems that the bit offset makes sense - it shifts the fixup bits to the right where they should be in little-endian order...

> I do believe we need to have a range check.   Doesn't common code already do that for fixups?   Might be good to check how other targets handle overflow here.

It seems the targets do that themselves in applyFixup() during handling of Value/FixupKind ("adjustFixupValue()"), so I added these checks as well for SystemZ. I suppose they should also be added for the PCXXDBL:s as well? Separately then I suppose might be best?

> The 20-bit displacements are always encoded the same way, that's why they can share the same relocation type.

right..

> Also, I think we should have more tests, e.g. testing both operands of the MVC-type instructions, testing at least one representative of each of the relevant insn formats (SSa-f, SSE, SSF).  We also should have tests both for the case where the fixup can be resolved, and where the fixup results in a relocation.

More tests added.


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

https://reviews.llvm.org/D113341

Files:
  llvm/lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp
  llvm/lib/Target/SystemZ/MCTargetDesc/SystemZInstPrinter.cpp
  llvm/lib/Target/SystemZ/MCTargetDesc/SystemZInstPrinter.h
  llvm/lib/Target/SystemZ/MCTargetDesc/SystemZMCAsmBackend.cpp
  llvm/lib/Target/SystemZ/MCTargetDesc/SystemZMCCodeEmitter.cpp
  llvm/lib/Target/SystemZ/MCTargetDesc/SystemZMCFixups.h
  llvm/lib/Target/SystemZ/MCTargetDesc/SystemZMCObjectWriter.cpp
  llvm/lib/Target/SystemZ/SystemZAsmPrinter.cpp
  llvm/lib/Target/SystemZ/SystemZInstrInfo.h
  llvm/test/MC/SystemZ/fixups-out-of-range.s
  llvm/test/MC/SystemZ/fixups.s
  llvm/test/MC/SystemZ/reloc-absolute.s
  llvm/test/MC/SystemZ/reloc-directive.s

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D113341.387256.patch
Type: text/x-patch
Size: 42502 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20211115/3b511269/attachment.bin>