[PATCH] D158197: [PowerPC][lld] Account for additional X-Forms -> D-Form/DS-Forms load/stores when relaxing initial-exec to local-exec

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added a comment.

This patch fixes a regression caused by a patch that is in v17. We will need to decide if we should backport this fix or if we should pull the original patch (which also has additional patches dependent on it that are in v17 as well). I think this patch is obvious enough to be backported, but I'll defer the final decision on that to @MaskRay once this is approved.
@amyk Perhaps it would help to perform a thorough test with the approved version of this patch applied to v17 to ensure it is safe to backport.



================
Comment at: lld/ELF/Arch/PPC64.cpp:855
+  default:
+    return 1;
+  }
----------------
I forgot to mention it yesterday, but I don't think we should have a different signal for failure here (i.e. returning 1 instead of 0 like in `getPPCDFormOp()`). Let's just use zero for both.


================
Comment at: lld/ELF/Arch/PPC64.cpp:944
+          error("unrecognized instruction for IE to LE R_PPC64_TLS");
+        finalInstr = (dFormOp | (read32(loc) & 0x03FFFFFC));
+        finalReloc = R_PPC64_TPREL16_LO_DS;
----------------
Using `0x03FFFFFC` rather than `0x03FFFFFF` is presumably because the two least significant bits of the DS-Form must not be overwritten. While I think that makes sense, I have a bit of a concern with using a different value there:
1. We always had DS-Forms there (`LD/STD`) and didn't treat them specially
2. Presumably, having either of those bits set in the incoming instruction is an indication that something has gone wrong with whatever produced the code, so masking out the bits just hides the issue

Perhaps it would be appropriate to set the `finalInstr` the same way regardless of how we set the `dFormOp` and just have an assert (or possibly a fatal_error or warning is even better) in the DS-Form section that those bits are unset.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158197