[PATCH] D108025: [JITLink] Unify x86-64 MachO and ELF 's optimize GOT/Stub function

[Lang Hames via Phabricator via llvm-commits]

lhames added a comment.

Hi Stephen,

Please address the inline comments before committing. Otherwise LGTM. Thank you very much for this work!

- Lang.



================
Comment at: llvm/lib/ExecutionEngine/JITLink/x86_64.cpp:76-79
+        if (E.getOffset() < 3 ||
+            strncmp(B->getContent().data() + E.getOffset() - 3,
+                    reinterpret_cast<const char *>(MOVQRIPRel), 2) != 0)
+          continue;
----------------
This should assert, rather than silently continuing -- a graph with an invalid edge is malformed.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/x86_64.cpp:92-93
+        int64_t Displacement = TargetAddr - EdgeAddr + 4;
+        if (Displacement >= std::numeric_limits<int32_t>::min() &&
+            Displacement <= std::numeric_limits<int32_t>::max()) {
+          // Change the edge kind as we don't go through GOT anymore. This is
----------------
This can use the `x86_64::isInRangeForImmS32` function now (the original optimize functions predate the utility, which is why it wasn't used before).


================
Comment at: llvm/lib/ExecutionEngine/JITLink/x86_64.cpp:105
+            dbgs() << "  Replaced GOT load wih LEA:\n    ";
+            printEdge(dbgs(), *B, E, getGenericEdgeKindName(E.getKind()));
+            dbgs() << "\n";
----------------
This should use x86_64::getEdgeKindName(E.getKind()).


================
Comment at: llvm/lib/ExecutionEngine/JITLink/x86_64.cpp:127-128
+        int64_t Displacement = TargetAddr - EdgeAddr + 4;
+        if (Displacement >= std::numeric_limits<int32_t>::min() &&
+            Displacement <= std::numeric_limits<int32_t>::max()) {
+          E.setKind(x86_64::BranchPCRel32);
----------------
This can also use `isInRangeForImmS32`.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/x86_64.cpp:133
+            dbgs() << "  Replaced stub branch with direct branch:\n    ";
+            printEdge(dbgs(), *B, E, getGenericEdgeKindName(E.getKind()));
+            dbgs() << "\n";
----------------
This should also use `x86_64::getEdgeKindName(E.getKind())`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108025