[JITLink][AArch32] Fixes for initial AArc32 backend

[Eymay]

Summary

• Fix masking error in Thumb_Jump24
• Fix halfword comparisons in asserts
• Add Data_Pointer32 to getEdgeKindName

[sgraenitz]

Thanks, good catches! I will double-check the conditional bit change and let you know.

[sgraenitz]

Ok, I finally double-checked it and you are right. Bit 12 must be set and not clear! Can we rename the field to LoBitUnconditional and change the condition into if (!(R.Lo & FixupInfo<Thumb_Jump24>::LoBitUnconditional))?

Additionally, I am afraid there is more to fix here. The docs say:

There is one relocation (R_ARM_CALL) for unconditional function call instructions (BLX and BL with the condition field set to 0xe), and one for jump instructions (R_ARM_JUMP24). [...] Conditional function call instructions (BL<cond>) must be relocated using R_ARM_JUMP24.

I assume the same holds for the respective Thumb instructions? (Do you know @peter.smith?) So, the Thumb_Jump24 relocation is the one that supports conditional branches and Thumb_Call is the one that should have this check :) I leave it to you whether or not to implement this now. We can also do it later and for now only rename the field and fix the condition. Everything else is fine. Thanks for working on this!

[sgraenitz]

In D157540#4636960, @sgraenitz wrote:

So, the Thumb_Jump24 relocation is the one that supports conditional branches and Thumb_Call is the one that should have this check

Just now I see that this is the bit that sets NoBlx for Thumb_Call and there is nothing to check here! We can simply remove the entire LoBitConditional check including the FixupInfo field :)

[peter.smith]

In D157540#4636960, @sgraenitz wrote:

Ok, I finally double-checked it and you are right. Bit 12 must be set and not clear! Can we rename the field to LoBitUnconditional and change the condition into if (!(R.Lo & FixupInfo<Thumb_Jump24>::LoBitUnconditional))?

Additionally, I am afraid there is more to fix here. The docs say:

There is one relocation (R_ARM_CALL) for unconditional function call instructions (BLX and BL with the condition field set to 0xe), and one for jump instructions (R_ARM_JUMP24). [...] Conditional function call instructions (BL<cond>) must be relocated using R_ARM_JUMP24.

I assume the same holds for the respective Thumb instructions? (Do you know @peter.smith?) So, the Thumb_Jump24 relocation is the one that supports conditional branches and Thumb_Call is the one that should have this check :) I leave it to you whether or not to implement this now. We can also do it later and for now only rename the field and fix the condition. Everything else is fine. Thanks for working on this!

Thumb (2) has some similarities and some differences. Like Arm there are two relocations R_ARM_THM_JUMP24 for B and a R_ARM_THM_CALL (BL and BLX). However in Thumb state no instructions are conditional they are made conditional by an IT block. So you can have something like:

        .text
        .thumb
        it eq
        bleq foo // R_ARM_THM_CALL

        .arm
        .type foo, %function
        .balign 4
foo:
        bx lr

Which at link time will end up as:

00008000 <$t.0>:
    8000: bf08          it      eq
    8002: f000 e802     blxeq   0x8008 <foo>            @ imm = #0x4
    8006: 0000          movs    r0, r0

00008008 <foo>:
    8008: e12fff1e      bx      lr

So in Thumb state the conditional bl can use the R_ARM_THM_CALL. Which it can't do in Arm state. I would recommend not generating a conditional BL in Arm state if possible as it means that if interworking is needed a full veneer/thunk/stub (whatever it is called in the tool) is needed.

[Eymay]

Removed unconditional check and combined commits into one.

[Eymay]

included both of the unconditional checks

llvm/include/llvm/ExecutionEngine/JITLink/aarch32.h

 ///   RegMask     - Mask with all bits set that encode the register
 ///
 template <EdgeKind_aarch32 Kind> struct FixupInfo {};
 
 template <> struct FixupInfo<Thumb_Jump24> {
   static constexpr HalfWords Opcode{0xf000, 0x8000};
   static constexpr HalfWords OpcodeMask{0xf800, 0x8000};
   static constexpr HalfWords ImmMask{0x07ff, 0x2fff};
-  static constexpr uint16_t LoBitConditional = 0x1000;
 };
 
 template <> struct FixupInfo<Thumb_Call> {
   static constexpr HalfWords Opcode{0xf000, 0xc000};
   static constexpr HalfWords OpcodeMask{0xf800, 0xc000};
   static constexpr HalfWords ImmMask{0x07ff, 0x2fff};
   static constexpr uint16_t LoBitH = 0x0001;
   static constexpr uint16_t LoBitNoBlx = 0x1000;

llvm/lib/ExecutionEngine/JITLink/aarch32.cpp

bool checkRegister(const ThumbRelocation &R, HalfWords Reg) {
   uint16_t Hi = R.Hi & FixupInfo<Kind>::RegMask.Hi;
   uint16_t Lo = R.Lo & FixupInfo<Kind>::RegMask.Lo;
   return Hi == Reg.Hi && Lo == Reg.Lo;
 }
 
 template <EdgeKind_aarch32 Kind>
 void writeRegister(WritableThumbRelocation &R, HalfWords Reg) {
   static constexpr HalfWords Mask = FixupInfo<Kind>::RegMask;
-  assert((Mask.Hi & Reg.Hi) == Reg.Hi && (Mask.Hi & Reg.Hi) == Reg.Hi &&
+  assert((Mask.Hi & Reg.Hi) == Reg.Hi && (Mask.Lo & Reg.Lo) == Reg.Lo &&
          "Value bits exceed bit range of given mask");
   R.Hi = (R.Hi & ~Mask.Hi) | Reg.Hi;
   R.Lo = (R.Lo & ~Mask.Lo) | Reg.Lo;
 }
 
 template <EdgeKind_aarch32 Kind>
 void writeImmediate(WritableThumbRelocation &R, HalfWords Imm) {
   static constexpr HalfWords Mask = FixupInfo<Kind>::ImmMask;
-  assert((Mask.Hi & Imm.Hi) == Imm.Hi && (Mask.Hi & Imm.Hi) == Imm.Hi &&
+  assert((Mask.Hi & Imm.Hi) == Imm.Hi && (Mask.Lo & Imm.Lo) == Imm.Lo &&
          "Value bits exceed bit range of given mask");
   R.Hi = (R.Hi & ~Mask.Hi) | Imm.Hi;
   R.Lo = (R.Lo & ~Mask.Lo) | Imm.Lo;
 }
 
 Expected<int64_t> readAddendData(LinkGraph &G, Block &B, const Edge &E) {
   support::endianness Endian = G.getEndianness();
   assert(Endian != support::native && "Declare as little or big explicitly");
if (!checkOpcode<Thumb_Call>(R))
       return makeUnexpectedOpcodeError(G, R, Kind);
     return LLVM_LIKELY(ArmCfg.J1J2BranchEncoding)
                ? decodeImmBT4BlT1BlxT2_J1J2(R.Hi, R.Lo)
                : decodeImmBT4BlT1BlxT2(R.Hi, R.Lo);
 
   case Thumb_Jump24:
     if (!checkOpcode<Thumb_Jump24>(R))
       return makeUnexpectedOpcodeError(G, R, Kind);
-    if (R.Lo & FixupInfo<Thumb_Jump24>::LoBitConditional)
-      return make_error<JITLinkError>("Relocation expects an unconditional "
-                                      "B.W branch instruction: " +
-                                      StringRef(G.getEdgeKindName(Kind)));
     return LLVM_LIKELY(ArmCfg.J1J2BranchEncoding)
                   ? decodeImmBT4BlT1BlxT2_J1J2(R.Hi, R.Lo)
                   : decodeImmBT4BlT1BlxT2(R.Hi, R.Lo);
 
   case Thumb_MovwAbsNC:
     if (!checkOpcode<Thumb_MovwAbsNC>(R))
       return makeUnexpectedOpcodeError(G, R, Kind);
     // Initial addend is interpreted as a signed value
Error applyFixupThumb(LinkGraph &G, Block &B, const Edge &E,
   uint64_t TargetAddress = TargetSymbol.getAddress().getValue();
   if (TargetSymbol.hasTargetFlags(ThumbSymbol))
     TargetAddress |= 0x01;
 
   switch (Kind) {
   case Thumb_Jump24: {
     if (!checkOpcode<Thumb_Jump24>(R))
       return makeUnexpectedOpcodeError(G, R, Kind);
-    if (R.Lo & FixupInfo<Thumb_Jump24>::LoBitConditional)
-      return make_error<JITLinkError>("Relocation expects an unconditional "
-                                      "B.W branch instruction: " +
-                                      StringRef(G.getEdgeKindName(Kind)));
     if (!(TargetSymbol.hasTargetFlags(ThumbSymbol)))
       return make_error<JITLinkError>("Branch relocation needs interworking "
                                       "stub when bridging to ARM: " +
                                       StringRef(G.getEdgeKindName(Kind)));
 
     int64_t Value = TargetAddress - FixupAddress + Addend;
     if (LLVM_LIKELY(ArmCfg.J1J2BranchEncoding)) {
       if (!isInt<25>(Value))

 
 const char *getEdgeKindName(Edge::Kind K) {
 #define KIND_NAME_CASE(K)                                                      \
   case K:                                                                      \
     return #K;
 
   switch (K) {
     KIND_NAME_CASE(Data_Delta32)
+    KIND_NAME_CASE(Data_Pointer32)
     KIND_NAME_CASE(Arm_Call)
     KIND_NAME_CASE(Thumb_Call)
     KIND_NAME_CASE(Thumb_Jump24)
     KIND_NAME_CASE(Thumb_MovwAbsNC)
     KIND_NAME_CASE(Thumb_MovtAbs)
   default:
     return getGenericEdgeKindName(K);
   }