[PATCH] D144083: [JITLink] Initial AArch32 backend

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

A few small comments concentrating on the Arm specifics.



================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/aarch32.h:96
+  bool HasMovtMovw = false;
+  bool J1J2BranchEncoding = false;
+  StubsFlavor Stubs = Unsupported;
----------------
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.  


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_aarch32.cpp:129
+      assert(Name && "We traversed section names before in graphifySections()");
+      bool NameIsArmEx = *Name == ".ARM.exidx" || *Name == ".ARM.extab";
+      assert(IsArmExSection == NameIsArmEx && "Let's see if this holds");
----------------
When -ffunction-sections is used clang will use `.ARM.exidx.function_name` and `.ARM.extab.function_name` . Not sure whether that will be the case for JITLink. If it is then it may be worth having these as prefix matches rather than a full match.


================
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");
+    });
----------------
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. 


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


================
Comment at: llvm/lib/ExecutionEngine/JITLink/aarch32.cpp:261
+                                      StringRef(G.getEdgeKindName(Kind)));
+    return decodeImmBT4BlT1BlxT2_J1J2(R.Hi, R.Lo);
+
----------------
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.


================
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)};
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144083