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

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand added a comment.

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

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

Ah, OK - even simpler.

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

OK, looks like this is correct.

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

Thanks!

> I suppose they should also be added for the PCXXDBL:s as well? Separately then I suppose might be best?

Agreed.

Otherwise this now LGTM - one minor nit inline.



================
Comment at: llvm/lib/Target/SystemZ/MCTargetDesc/SystemZMCAsmBackend.cpp:162
+    uint64_t DHi = (Value >> 12) & 0xff;
+    Value = (DLo << 8) | DHi;
+  }
----------------
I think it might be preferabe to do the bit-swapping **in** `extractBitsForFixup` - that routine is supposed to return the //bits// to be placed in the instruction slot.


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

https://reviews.llvm.org/D113341