[ORC] Drop Comdat when discarding IR symbol

[Hahnfeld]

Summary

According to the IR verifier, "Declaration[s] may not be in a Comdat!"

[sunho]

What happens when we actually have duplicate comdat any definitions? IIRC I've seen that in msvcrt.lib.

[Hahnfeld]

In D142443#4077610, @sunho wrote:

What happens when we actually have duplicate comdat any definitions? IIRC I've seen that in msvcrt.lib.

I'm not sure I understand your question. I think this is partially what https://reviews.llvm.org/D138264 is about? De-duplicating Comdat symbols on Linux doesn't seem to work as of now, hence the weak definition in weak-comdat.ll (but I'm not really into this Comdat business, not sure if a non-weak definition should work there).

[Hahnfeld]

ping @lhames

[sunho]

Sorry for the delay. I was only looking at the test case; the code change looks reasonable to me. As they are supposed to be discarded anyways, it's safe to just remove COMDAT data as well to make IR valid.

Testcases should trigger a call to discard function since weak symbol gets overridden, right?

[Hahnfeld]

In D142443#4098212, @sunho wrote:

Testcases should trigger a call to discard function since weak symbol gets overridden, right?

Yes, the test triggers exactly this condition. With current main and using a build with assertions, you should see the IR verifier complain before code generation.

[sunho]

LGTM for me. That validation error has been a problem on Windows too.

[Hahnfeld]

In D142443#4098847, @sunho wrote:

LGTM for me. That validation error has been a problem on Windows too.

It's exactly for that purpose ;-) https://github.com/root-project/root/pull/12074

[spatel]

The test seems to be crashing on macOS with or without the code change:
"LLVM ERROR: MachO doesn't support COMDATs, 'f' cannot be lowered."

[Hahnfeld]

In D142443#4102513, @spatel wrote:

The test seems to be crashing on macOS with or without the code change:
"LLVM ERROR: MachO doesn't support COMDATs, 'f' cannot be lowered."

Hm right... If you are on macOS, could you test if XFAIL: target={{.*}}-darwin{{.*}} correctly excludes the test?

[spatel]

In D142443#4102519, @Hahnfeld wrote:

In D142443#4102513, @spatel wrote:

The test seems to be crashing on macOS with or without the code change:
"LLVM ERROR: MachO doesn't support COMDATs, 'f' cannot be lowered."

Hm right... If you are on macOS, could you test if XFAIL: target={{.*}}-darwin{{.*}} correctly excludes the test?

Still crashing after I add that line - let me know if I'm missing anything:

diff --git a/llvm/test/ExecutionEngine/Orc/weak-comdat.ll b/llvm/test/ExecutionEngine/Orc/weak-comdat.ll
index 8cedd57ef922..7195f5a2b00e 100644
--- a/llvm/test/ExecutionEngine/Orc/weak-comdat.ll
+++ b/llvm/test/ExecutionEngine/Orc/weak-comdat.ll
@@ -1,4 +1,5 @@
 ; RUN: lli -extra-module %p/Inputs/weak-comdat-def.ll %s
+; XFAIL: target={{.*}}-darwin-{{.*}}
 
 declare i32 @g()

[Hahnfeld]

In D142443#4102559, @spatel wrote:

In D142443#4102519, @Hahnfeld wrote:

In D142443#4102513, @spatel wrote:

The test seems to be crashing on macOS with or without the code change:
"LLVM ERROR: MachO doesn't support COMDATs, 'f' cannot be lowered."

Hm right... If you are on macOS, could you test if XFAIL: target={{.*}}-darwin{{.*}} correctly excludes the test?

Still crashing after I add that line - let me know if I'm missing anything:

Yeah sorry, I added an additional dash after darwin in my initial comment. Can you try removing that?

[spatel]

In D142443#4102581, @Hahnfeld wrote:

In D142443#4102559, @spatel wrote:

In D142443#4102519, @Hahnfeld wrote:

In D142443#4102513, @spatel wrote:

The test seems to be crashing on macOS with or without the code change:
"LLVM ERROR: MachO doesn't support COMDATs, 'f' cannot be lowered."

Hm right... If you are on macOS, could you test if XFAIL: target={{.*}}-darwin{{.*}} correctly excludes the test?

Still crashing after I add that line - let me know if I'm missing anything:

Yeah sorry, I added an additional dash after darwin in my initial comment. Can you try removing that?

Yes - that works; no more crash report.

[thakis]

This breaks tests on Mac: http://45.33.8.238/macm1/54167/step_11.txt

Please take a look and revert for now if it takes a while to fix.

[Hahnfeld]

@spatel thanks for testing, now pushed as rGde7e9ebe46d1acc0ee12609ada49aa21064b7a97

@thakis please take a look at the previous five comments, it was already under investigation. Although I have to say it's weird that I received zero emails from the bots...

[thakis]

Instead of adding an xfail, does it make more sense to pass an explicit triple to lli instead?

[lhames]

In D142443#4103013, @thakis wrote:

Instead of adding an xfail, does it make more sense to pass an explicit triple to lli instead?

I think UNSUPPORTED is the right way to go here. I've made that change in 7e528d4689e6.

Belatedly -- this change looks good to me. Thanks @Hahnfeld!

[Hahnfeld]

I received three complaints from AArch64 bots now failing Clang Interpreter tests, for example: https://lab.llvm.org/buildbot/#/builders/197/builds/3920 I don't quite understand how this can happen and currently have no easy way to verify, so reverting a second time...

[lhames]

In D142443#4104430, @Hahnfeld wrote:

I received three complaints from AArch64 bots now failing Clang Interpreter tests, for example: https://lab.llvm.org/buildbot/#/builders/197/builds/3920 I don't quite understand how this can happen and currently have no easy way to verify, so reverting a second time...

That looks like the issue that's being discussed in https://reviews.llvm.org/D143039. If so then it's unrelated to your patch.

[Hahnfeld]

In D142443#4104625, @lhames wrote:

In D142443#4104430, @Hahnfeld wrote:

I received three complaints from AArch64 bots now failing Clang Interpreter tests, for example: https://lab.llvm.org/buildbot/#/builders/197/builds/3920 I don't quite understand how this can happen and currently have no easy way to verify, so reverting a second time...

That looks like the issue that's being discussed in https://reviews.llvm.org/D143039. If so then it's unrelated to your patch.

Yes, I noticed shortly after my revert that the tests had actually turned green again after 5b2549b0d2834a653be60b52885bdc9f21abc2ee. I will push shortly, folding in your change to UNSUPPORTED.

llvm/lib/ExecutionEngine/Orc/Layer.cpp

void IRMaterializationUnit::discard(const JITDylib &JD,
   }););
 
   auto I = SymbolToDefinition.find(Name);
   assert(I != SymbolToDefinition.end() &&
          "Symbol not provided by this MU, or previously discarded");
   assert(!I->second->isDeclaration() &&
          "Discard should only apply to definitions");
   I->second->setLinkage(GlobalValue::AvailableExternallyLinkage);
+  // According to the IR verifier, "Declaration[s] may not be in a Comdat!"
+  // Remove it, if this is a GlobalObject.
+  if (auto *GO = dyn_cast<GlobalObject>(I->second))
+    GO->setComdat(nullptr);
   SymbolToDefinition.erase(I);
 }
 
 BasicIRLayerMaterializationUnit::BasicIRLayerMaterializationUnit(
     IRLayer &L, const IRSymbolMapper::ManglingOptions &MO, ThreadSafeModule TSM)
     : IRMaterializationUnit(L.getExecutionSession(), MO, std::move(TSM)), L(L) {
 }
 

llvm/test/ExecutionEngine/Orc/Inputs/weak-comdat-def.ll

+$c = comdat any
+
+define i32 @f() comdat($c) {
+entry:
+  ret i32 0
+}

llvm/test/ExecutionEngine/Orc/weak-comdat.ll

+; RUN: lli -extra-module %p/Inputs/weak-comdat-def.ll %s
+
+$c = comdat any
+
+define weak i32 @f() comdat($c) {
+entry:
+  ret i32 0
+}
+
+define i32 @main() {
+entry:
+  %0 = call i32 @f()
+  ret i32 %0
+}