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

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand added a comment.

In D113341#3126096 <https://reviews.llvm.org/D113341#3126096>, @jonpa wrote:

> - Mapping of MI:OperandIndex to fixup byte offset: I added a TSFlags bit 'MemMem' which is checked for in getDispOpValue(). From this flag it is known that there are two memory operands, with displacements starting at bit 20 and 36. For the matter of choosing which one of these are in question in getDispOpValue(), based on the OpNum, I was not sure if this could be hard coded as well in the formats file, or if it would be simpler to check this at compile time the way that is currently done..?

There's a number of options I can see, each of which has their own drawbacks.

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.
2. We could follow the existing precedent with the pc-relative operands that the operand type encodes the byte offset.  This means that for the MemMem operands, we'd have to use two different operand types.  So in addition to `bdaddr12only` we'd have another type like `bdaddr12second`  that would work the same way everywhere, except that the first type uses a byte offset of 2 for relocations while the second type uses a byte offset of 4.  (We'd likewise need variants of `bdaddr12onlylen4` and `bdaddr12onlylen8`, I think.)  Then we can explicitly encode in each insn pattern where to use `bdaddr12only` and where to use `bdaddr12second`.
3. Ideally, TableGen should be able to handle this for us.   It actually already knows exactly which bits the results of the operand encoding will end up at, because it needs to place them there.  If we could get TableGen to pass that information (e.g. the mask and shift count) into the Encoding callback as well, then we wouldn't need to duplicate that information in the first place.  See the generated `SystemZMCCodeEmitter::getBinaryCodeForInstr` function:

  // op: BDL1
  op = getBDLAddr12Len8Encoding(MI, 0, Fixups, STI);
  op &= UINT64_C(16777215);
  op <<= 16;
  Value |= op;
  // op: BD2
  op = getBDAddr12Encoding(MI, 3, Fixups, STI);
  op &= UINT64_C(65535);
  Value |= op;
  break;

I guess I'd be OK with an implementation along any of these lines.

> - Instead of in the AsmParser allowing specifically an MCSymbolRefExpr as displacement (which does not work with 'b-a'), the patch now allows any MCExpr if a symbol allowed as displacement. It seems unrelocatable expressions are caught by evaluateAsRelocatable().

That seems correct to me.

> - Removed previous changes in applyFixup(). It seems the bit offset is actually not needed there during the insertion of reversed bytes... Test added for this as well (reloc-absolute.s). Is there a need for a range check here (for a too-big displacement)? Handle the value for a FK_390_20 displacement by putting the high byte first - I hope that holds true for all uses...?

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

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

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.


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

https://reviews.llvm.org/D113341