This is an archive of the discontinued LLVM Phabricator instance.

[ORC] Drop Comdat when discarding IR symbol
ClosedPublic

Authored by Hahnfeld on Jan 24 2023, 1:57 AM.

Diff Detail

Event Timeline

Hahnfeld created this revision.Jan 24 2023, 1:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 24 2023, 1:57 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
Hahnfeld requested review of this revision.Jan 24 2023, 1:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 24 2023, 1:57 AM
sunho added a comment.Jan 24 2023, 9:11 AM

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

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).

sunho added a comment.Feb 1 2023, 5:09 PM

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?

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 accepted this revision.Feb 2 2023, 1:04 AM

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

This revision is now accepted and ready to land.Feb 2 2023, 1:04 AM

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

This revision was landed with ongoing or failed builds.Feb 2 2023, 11:42 PM
This revision was automatically updated to reflect the committed changes.
spatel added a subscriber: spatel.Feb 3 2023, 6:52 AM

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 added a comment.EditedFeb 3 2023, 6:55 AM

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 added a comment.EditedFeb 3 2023, 7:05 AM

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()

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 added a comment.Feb 3 2023, 7:24 AM

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 added a subscriber: thakis.Feb 3 2023, 7:25 AM

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.

@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...

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

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 reopened this revision.Feb 4 2023, 12:28 PM

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...

This revision is now accepted and ready to land.Feb 4 2023, 12:28 PM
lhames added a comment.Feb 4 2023, 9:15 PM

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.

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.

This revision was automatically updated to reflect the committed changes.