[PATCH] D130479: [ORC_RT][COFF] Initial platform support for COFF/x86_64.

[Lang Hames via Phabricator via llvm-commits]

lhames added inline comments.


================
Comment at: compiler-rt/test/orc/TestCases/Windows/x86-64/lit.local.cfg.py:1-4
+if config.root.host_arch not in ['AMD64','x86_64']:
+  config.unsupported = True
+
+if config.target_arch not in ['AMD64','x86_64']:
----------------
sunho wrote:
> lhames wrote:
> > Can this just be `config.target_arch != "x86_64"`? Do we not canonicalize AMD64 -> x86_64?
> Not exactly sure what's going on here. On my local machine, only AMD64 works.
//Huh//. I wonder if that's intended? I'm not curious enough to hold this patch up while I investigate though. Let's stick with what you have. :)


================
Comment at: llvm/lib/ExecutionEngine/Orc/COFFPlatform.cpp:386-425
+Expected<COFFPlatform::JITDylibDepMap>
+COFFPlatform::buildJDDepMap(JITDylib &JD) {
+  return ES.runSessionLocked([&]() -> Expected<JITDylibDepMap> {
+    JITDylibDepMap JDDepMap;
+
+    SmallVector<JITDylib *, 16> Worklist({&JD});
+    while (!Worklist.empty()) {
----------------
sunho wrote:
> lhames wrote:
> > This is the same algorithm as MachO, right? We should look for a way to factor this out, but that can happen in a followup patch.
> I thought the same while doing this. What's more urgent in my opinion is refactor the orc runtime side code to share more code. A lot of code is duplicated across multiple platforms there. 
Yeah -- let's get this landed and build some experience with it, and then what we learn can feed into a refactor to share common code.


================
Comment at: llvm/tools/llvm-jitlink/llvm-jitlink.cpp:1222-1223
 
+Expected<JITDylib *> Session::getOrLoadDynamicLibrary(StringRef LibName,
+                                                      StringRef LibPath) {
+  auto It = DynLibJDs.find(LibName.str());
----------------
sunho wrote:
> lhames wrote:
> > Is there any reason to make a distinction between `LibName` and `LibPath` here? Could we just use the path as the name?
> It's separated because same DLL can exist in multiple include pathes. All the path stuffs is done to make sure we don't double import the same DLL.
Are DLLs identified by filename alone then? Or is there some other way to tell whether two DLLs represent the same logical library?


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

https://reviews.llvm.org/D130479