[PATCH] D144083: [JITLink] Initial AArch32 backend

[Stefan Gränitz via Phabricator via llvm-commits]

sgraenitz added a comment.

Thanks for your notes! I followed up on a few and will try to post a patch towards the end of the week.



================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/aarch32.h:96
+  bool HasMovtMovw = false;
+  bool J1J2BranchEncoding = false;
+  StubsFlavor Stubs = Unsupported;
----------------
peter.smith wrote:
> At the moment this patch supports Arm v7 and V8. The J1J2 branch encoding is present in all v7 and v8. A rough rule of Thumb is All Cortex cores v6-m v7, v8 etc. and the Arm1156t2-S (Thumb-2's introduction). If older CPUs that don't support Thumb-2 are quite far down the roadmap it may be worth simplifying the patch by not including this.
> 
> This is just a suggestion, I can see that it is unit tested so no objections to leaving it in.  
Ok, I see. So far I used this to look how passing through an extra config will work out, since the other backends don't do it and here it looks like we need at least some kind of config. I will remove the unused fields.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_aarch32.cpp:130
+      bool NameIsArmEx = *Name == ".ARM.exidx" || *Name == ".ARM.extab";
+      assert(IsArmExSection == NameIsArmEx && "Let's see if this holds");
+    });
----------------
peter.smith wrote:
> This will hold for .ARM.exidx sections, but not for .ARM.extab. The .ARM.extab sections are only ever referenced from .ARM.exidx sections (when the number of unwind instructions is too big to inline into 4-bytes they need to go into a separate section.
> ```
> .text <- R_ARM_PREL31 (SHF_LINK_ORDER) .ARM.exidx -> R_ARM_PREL31 .ARM.extab
> ```
> 
> The .ARM.exidx sections have a specific ELF type SHT_ARM_EXIDX (https://github.com/ARM-software/abi-aa/blob/main/aaelf32/aaelf32.rst#532section-types) but .ARM.extab does not. 
Ah right, I only checked the flags, but I should use `sh_type` thanks.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/aarch32.cpp:244
+      return makeUnexpectedOpcodeError(G, R, Kind);
+    if ((R.Lo & FixupInfo<Thumb_Call>::LoBitNoBlx) == 0 &&
+        (R.Lo & FixupInfo<Thumb_Call>::LoBitH) != 0)
----------------
peter.smith wrote:
> Looking at the stub generation code, if there is a Thumb_Call to an external edge, then a stub with LoBitH will be set will be created. This won't be a problem if the code-generator never generates BLX to an external symbol, but would be if it does.
Yes, I didn't catch a case like this yet. But I am also not sure I fully understand: If codegen wrote a BLX, then it would be T2 right? The docs say LoBitH must be empty here:
```
if CurrentInstrSet() == InstrSet_ThumbEE || H == '1' then UNDEFINED;
```

What is the stub generation code you mean? The one here in JITLink?


================
Comment at: llvm/lib/ExecutionEngine/JITLink/aarch32.cpp:261
+                                      StringRef(G.getEdgeKindName(Kind)));
+    return decodeImmBT4BlT1BlxT2_J1J2(R.Hi, R.Lo);
+
----------------
peter.smith wrote:
> Is there a reason why this isn't
> ```
> LLVM_LIKELY(ArmCfg.J1J2BranchEncoding)
>                ? decodeImmBT4BlT1BlxT2_J1J2(R.Hi, R.Lo)
>                : decodeImmBT4BlT1BlxT2(R.Hi, R.Lo);
> ```
> The J1J2 would apply here in the same way as Thumb_Call.
My understanding was that CPUs without J1J2 didn't have R_ARM_THM_JUMP24 yet, so it wouldn't every happen? (Maybe that would be worth an assertion.)


================
Comment at: llvm/unittests/ExecutionEngine/JITLink/AArch32Tests.cpp:71
+template <endianness Endian>
+static HalfWords makeHalfWords(std::array<uint8_t, 4> Mem) {
+  return HalfWords{read16<Endian>(Mem.data()), read16<Endian>(Mem.data() + 2)};
----------------
peter.smith wrote:
> Big-endian for v7 and v8 (and v6 unless in legacy backwards compatible mode be32) have little-endian instructions and big-endian data. If this is just for instructions you probably don't need the template parameter.
> 
> You'll still see big-endian instructions in ELF relocatable objects, a be8 supporting linker is expected to endian-reverse instructions for the executable.
Somehow I wanted to make this explicit. It's probably worth a comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144083