[ORC] Implement basic reoptimization. #67050

sunho · 2023-09-21T18:47:16Z

No description provided.

lhames

Just reviewing the RedirectionManager interface and implementation patch for now. I have some minor comments, but otherwise this is looking amazing -- thank you very much @sunho!

lhames · 2023-09-23T04:49:22Z

llvm/include/llvm/ExecutionEngine/Orc/RedirectionManager.h

+    for (auto [K, V] : InitialDests) {
+      Flags[K] = V.getFlags();
+    }


No braces needed for single-line loops. :)

lhames · 2023-09-23T04:51:17Z

llvm/lib/ExecutionEngine/Orc/RedirectionManager.cpp

+  if (auto Err = JD.define(std::make_unique<RedirectableMaterializationUnit>(
+                               *this, InitialDests),
+                           RT))
+    return Err;
+
+  return Error::success();


This could just be:

return JD.define(std::make_unique<RedirectableMaterializationUnit>(*this, InitialDests), RT);

lhames · 2023-09-23T04:57:15Z

llvm/lib/ExecutionEngine/Orc/JITLinkRedirectableSymbolManager.cpp

+      R->failMaterialization();
+      return;
+    }
+    dbgs() << *K << "\n";


Stray debugging output?

lhames · 2023-09-23T05:04:31Z

llvm/lib/ExecutionEngine/Orc/JITLinkRedirectableSymbolManager.cpp

+  if (auto Err = R->replace(absoluteSymbols(NewSymbolDefs))) {
+    ES.reportError(std::move(Err));
+    R->failMaterialization();
+    return;
+  }
+
+  if (auto Err = redirectInner(TargetJD, InitialDests)) {
+    ES.reportError(std::move(Err));
+    R->failMaterialization();
+    return;
+  }


I think the call to redirectInner should precede the call to replace: As soon as replace runs anyone waiting on NewSymbolDefs will be notified that those symbols are available, but they're not actually usable until the call to redirectInner, right?

You should also return the stubs to the pool on all three of the failure paths (or add a FIXME that we should do that in the future).

lhames · 2023-09-23T05:05:48Z

llvm/lib/ExecutionEngine/Orc/JITLinkRedirectableSymbolManager.cpp

+  if (auto Err = ES.getExecutorProcessControl().getMemoryAccess().writePointers(
+          PtrWrites))
+    return Err;
+  return Error::success();


This could just be:

return ES.getExecutorProcessControl().getMemoryAccess().writePointers(PtrWrites);

lhames · 2023-09-23T05:13:38Z

llvm/lib/ExecutionEngine/Orc/JITLinkRedirectableSymbolManager.cpp

+  SymbolLookupSet LookupSymbols;
+  DenseMap<SymbolStringPtr, ExecutorSymbolDef *> NewDefsMap;
+
+  Triple TT = ES.getTargetTriple();
+  auto G = std::make_unique<jitlink::LinkGraph>(
+      "<INDIRECT STUBS>", TT, TT.isArch64Bit() ? 8 : 4,
+      TT.isLittleEndian() ? support::little : support::big,
+      jitlink::getGenericEdgeKindName);
+  auto &PointerSection =
+      G->createSection(StubPtrTableName, MemProt::Write | MemProt::Read);
+  auto &StubsSection =
+      G->createSection(JumpStubTableName, MemProt::Exec | MemProt::Read);
+
+  for (size_t I = OldSize; I < NewSize; I++) {
+    auto Pointer = AnonymousPtrCreator(*G, PointerSection, nullptr, 0);
+    if (auto Err = Pointer.takeError())
+      return Err;
+
+    StringRef PtrSymName = StubPtrSymbolName(I);
+    Pointer->setName(PtrSymName);
+    Pointer->setScope(jitlink::Scope::Default);
+    LookupSymbols.add(ES.intern(PtrSymName));
+    NewDefsMap[ES.intern(PtrSymName)] = &StubPointers[I];
+
+    auto Stub = PtrJumpStubCreator(*G, StubsSection, *Pointer);
+    if (auto Err = Stub.takeError())
+      return Err;
+
+    StringRef JumpStubSymName = JumpStubSymbolName(I);
+    Stub->setName(JumpStubSymName);
+    Stub->setScope(jitlink::Scope::Default);
+    LookupSymbols.add(ES.intern(JumpStubSymName));
+    NewDefsMap[ES.intern(JumpStubSymName)] = &JumpStubs[I];
+  }


We only need one anchor symbol to trigger emission of the graph. Eventually I think this could be sped up by emitting:

.section <JumpStubTableName> __<JumpStubTableName>.<++Idx>: ... stubs ... .section <StubPtrTableName> __<StubPtrTableName>.<Idx>: ... ptrs ...

Then looking up __JumpStubTableName.<Idx> and __StubPtrTableName.<Idx> and using the stub and pointer sizes to stride through the resulting addresses.

I don't think this change needs to happen before the commit though -- you could add a FIXME and this can be handled in the future.

lhames · 2023-09-24T04:28:47Z

llvm/include/llvm/ExecutionEngine/Orc/IRPartitionLayer.h

+namespace llvm {
+namespace orc {
+
+class IRPartitionLayer : public IRLayer {


It'd be good to add a class comment explaining the layer's behavior. (I know the file comment covers this, but people looking up the type docs may not see that).

… JITLink.

llvm/include/llvm/ExecutionEngine/Orc/RedirectionManager.h

llvm/include/llvm/ExecutionEngine/Orc/JITLinkRedirectableSymbolManager.h

lhames · 2024-04-13T22:13:14Z

llvm/include/llvm/ExecutionEngine/Orc/JITLinkRedirectableSymbolManager.h

+#ifndef LLVM_EXECUTIONENGINE_ORC_JITLINKREDIRECABLEMANAGER_H
+#define LLVM_EXECUTIONENGINE_ORC_JITLINKREDIRECABLEMANAGER_H


Include guard should be LLVM_EXECUTIONENGINE_ORC_JITLINKREDIRECABLESYMBOLMANAGER_H.

lhames

Minor comments aside: LGTM!

lhames · 2024-04-13T23:17:05Z

llvm/include/llvm/ExecutionEngine/Orc/ReOptimizeLayer.h

+#ifndef LLVM_EXECUTIONENGINE_ORC_REOPTLAYER_H
+#define LLVM_EXECUTIONENGINE_ORC_REOPTLAYER_H


Include guard should be LLVM_EXECUTIONENGINE_ORC_REOPTIMIZELAYER_H to match the file name.

lhames · 2024-04-13T23:23:53Z

llvm/include/llvm/ExecutionEngine/Orc/ReOptimizeLayer.h

+namespace llvm {
+namespace orc {
+
+using ReOptMaterializationUnitID = uint64_t;


Use of this type seems to be contained within the ReOptimizeLayer -- could it be a member of that class?

This reverts commit 0d288e5. Breaks the build.

nikic · 2024-04-26T05:48:57Z

I've reverted this to fix build failures -- it looks like this has been implemented against a very old LLVM version.

sunho · 2024-04-26T05:50:26Z

Thanks for reverting the commit. Let me fix build error shortly.

JPMMaia · 2024-05-01T16:09:05Z

llvm/include/llvm/ExecutionEngine/Orc/ReOptimizeLayer.h

+  /// Registers reoptimize runtime dispatch handlers to given PlatformJD. The
+  /// reoptimization request will not be handled if dispatch handler is not
+  /// registered by using this function.
+  Error reigsterRuntimeFunctions(JITDylib &PlatformJD);


Typo: reigsterRuntimeFunctions -> registerRuntimeFunctions

Thrrreeee · 2024-06-05T05:17:51Z

Thanks for reverting the commit. Let me fix build error shortly.

Hi @sunho，
Could you please let me know how the progress on the code modification is going and approximately how much longer it will take to complete?

sunho · 2024-06-07T01:02:54Z

@Thrrreeee Oh this has been forgotten. It should be a trivial fix. Let me try to reland it tmr.

Thrrreeee · 2024-06-10T08:37:18Z

Hi @sunho,
I have some questions. For example, in the reoptimization layer, we need to obtain profile information. How can we profile JITed code and then perform PGO? It doesn't seem like this is the case with the ORC JIT yet.
I know that there are JITlisntener to support Perf to profile JITed code, but how can we use it in PRC JIT? Dynamic instrumentation or use other profiler to get profile information?Could you tell me your plan?

Thrrreeee · 2024-06-16T06:53:35Z

Let me try to reland it tmr.

Hi @sunho, Have you finished this?

sunho · 2024-10-11T16:41:39Z

Some build bots are breaking because we are running runtime JIT execution unit tests on unsupported targets. Investigating this issue right now.

sunho · 2024-10-11T17:22:02Z

Relanded on 188ede2

sunho · 2024-10-12T06:58:02Z

Failing at https://lab.llvm.org/buildbot/#/builders/161/builds/2692. Probably wise to disable to test on the COFF-ARM64.

llvmbot added clang Clang issues not falling into any other category compiler-rt clang:codegen labels Sep 21, 2023

sunho requested a review from lhames September 21, 2023 20:17

lhames requested changes Sep 23, 2023

View reviewed changes

lhames reviewed Sep 24, 2023

View reviewed changes

sunho added 2 commits December 18, 2023 18:36

[ORC] Introduce RedirectionManager interface and implementation using…

7f4db32

… JITLink.

[ORC] Introduce IRPartitionLayer for common partition functionality.

6059562

sunho force-pushed the sitai branch from 342adf1 to 044695d Compare December 19, 2023 02:37

lhames requested changes Apr 6, 2024

View reviewed changes

llvm/include/llvm/ExecutionEngine/Orc/RedirectionManager.h Show resolved Hide resolved

llvm/include/llvm/ExecutionEngine/Orc/JITLinkRedirectableSymbolManager.h Outdated Show resolved Hide resolved

sunho force-pushed the sitai branch from 044695d to 6b5eac2 Compare April 11, 2024 04:42

This was referenced Apr 11, 2024

[ORC] Introduce IRPartitionLayer for common partition functionality. #66812

Closed

[ORC] Introduce RedirectionManager interface and implementation using JITLink. #66802

Closed

lhames reviewed Apr 13, 2024

View reviewed changes

lhames approved these changes Apr 14, 2024

View reviewed changes

[ORC] Implement basic reoptimization.

d22243c

sunho force-pushed the sitai branch from 6b5eac2 to d22243c Compare April 26, 2024 00:41

sunho merged commit 0d288e5 into llvm:main Apr 26, 2024
3 of 4 checks passed

nikic added a commit that referenced this pull request Apr 26, 2024

Revert "[ORC] Implement basic reoptimization. (#67050)"

47682e4

This reverts commit 0d288e5. Breaks the build.

JPMMaia reviewed May 1, 2024

View reviewed changes

sunho referenced this pull request Oct 11, 2024

[ORC] Implement basic reoptimization.

188ede2

tstellar mentioned this pull request Oct 17, 2024

Snapshot for 20241013, v20.0.0, 464a7ee (big-merge) fedora-llvm-team/llvm-snapshots#792

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ORC] Implement basic reoptimization. #67050

[ORC] Implement basic reoptimization. #67050

sunho commented Sep 21, 2023

lhames left a comment

lhames Sep 23, 2023

lhames Sep 23, 2023

lhames Sep 23, 2023

lhames Sep 23, 2023

lhames Sep 23, 2023

lhames Sep 23, 2023

lhames Sep 24, 2023

lhames Apr 13, 2024

lhames left a comment

lhames Apr 13, 2024

lhames Apr 13, 2024

nikic commented Apr 26, 2024

sunho commented Apr 26, 2024

JPMMaia May 1, 2024

Thrrreeee commented Jun 5, 2024

sunho commented Jun 7, 2024

Thrrreeee commented Jun 10, 2024

Thrrreeee commented Jun 16, 2024 •

edited

Loading

sunho commented Oct 11, 2024

sunho commented Oct 11, 2024

sunho commented Oct 12, 2024 •

edited

Loading

		#ifndef LLVM_EXECUTIONENGINE_ORC_JITLINKREDIRECABLEMANAGER_H
		#define LLVM_EXECUTIONENGINE_ORC_JITLINKREDIRECABLEMANAGER_H

		#ifndef LLVM_EXECUTIONENGINE_ORC_REOPTLAYER_H
		#define LLVM_EXECUTIONENGINE_ORC_REOPTLAYER_H

[ORC] Implement basic reoptimization. #67050

[ORC] Implement basic reoptimization. #67050

Conversation

sunho commented Sep 21, 2023

lhames left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhames left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikic commented Apr 26, 2024

sunho commented Apr 26, 2024

Choose a reason for hiding this comment

Thrrreeee commented Jun 5, 2024

sunho commented Jun 7, 2024

Thrrreeee commented Jun 10, 2024

Thrrreeee commented Jun 16, 2024 • edited Loading

sunho commented Oct 11, 2024

sunho commented Oct 11, 2024

sunho commented Oct 12, 2024 • edited Loading

Thrrreeee commented Jun 16, 2024 •

edited

Loading

sunho commented Oct 12, 2024 •

edited

Loading