[PATCH] D144083: [JITLink] Initial 32-bit ARM backend

[Lang Hames via Phabricator via llvm-commits]

lhames added a comment.

Very cool. Thank you for contributing this -- it's great to finally see ARM32 support in JITLink! :)



================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/JITLink.h:473
+    //
+    assert(((Offset & ~0x1) + Size) <= Base.getSize() &&
            "Symbol extends past end of block");
----------------
sgraenitz wrote:
> @lhames Ideas how to get this cleaner?
I wonder if any other architectures use the low bits of addresses for architecture specific flags? Maybe we could come up with something more general, like a triple-specific 'getRawAddressForArithmetic` function that we could use in common code.

At a minimum we could make the test in this assert conditional on the graph having a thumb triple -- that way we wouldn't affect other platforms.


================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/arm.h:97-105
+inline int64_t getPCBias(Edge::Kind K) {
+  switch (K) {
+  case Thumb_Call:
+  case Thumb_Jump24:
+    return 4;
+  default:
+    return 8;
----------------
I think we should spell out the relocations with bias `8` and have the default case be an `llvm_unreachable`, just to be safe.


================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/arm.h:238
+                 uint64_t Alignment) {
+    ArrayRef<char> Template(reinterpret_cast<const char *>(Code), Size);
+    return G.createContentBlock(getStubsSection(G), Template,
----------------
As an aside to this review -- I find myself doing this a lot too. I wonder if we should add some `ArrayRef<char> <-> ArrayRef<uint8_t>` conversion helpers in ADT?

I've also gone back and forward on whether block content should be `ArrayRef<char>` or `ArrayRef<uint8_t>`. From memory they were originally `uint8_t`-based and I moved them to `char` at some point in the hope of reducing friction, but I'm not sure it was very successful in the end. Neither type seems to be a perfect fit.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELFLinkGraphBuilder.h:113
+  /// graph.
+  virtual bool excludeSectionByName(StringRef Name) const { return false; }
+
----------------
Is there an ELF attribute or type that we could be looking at, rather than the name?

(I'm very guilty of using section names rather than attributes in checks, but as JITLink evolves I do want us to use attributes and types more where possible)


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_arm.cpp:180-197
+  case Pre_v4:
+  case v4:
+  case v4T:
+  case v5T:
+  case v5TE:
+  case v5TEJ:
+  case v6:
----------------
Should this just be the `default` case, to future-proof against new ArchKinds? (Not sure whether we're expecting any at this point?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144083