Revert "[MC] Explicitly mark MCSymbol for MO_ExternalSymbol" #133291
Conversation
This reverts commit 3d0846b.
|
@llvm/pr-subscribers-backend-x86 Author: Eli Friedman (efriedma-quic) ChangesReverts llvm/llvm-project#108880 . The patch has no regression test, no description of why the fix is necessary, and the code is modifying MC datastructures in a way that's forbidden in the AsmPrinter. And the author didn't respond to a post-commit review comment requesting a regression test. Fixes #132055. Full diff: https://github.com/llvm/llvm-project/pull/133291.diff 1 Files Affected:
diff --git a/llvm/lib/Target/X86/X86MCInstLower.cpp b/llvm/lib/Target/X86/X86MCInstLower.cpp
index 3bd012c13cf0d..3f6cd55618666 100644
--- a/llvm/lib/Target/X86/X86MCInstLower.cpp
+++ b/llvm/lib/Target/X86/X86MCInstLower.cpp
@@ -349,12 +349,8 @@ MCOperand X86MCInstLower::LowerMachineOperand(const MachineInstr *MI,
return MCOperand::createImm(MO.getImm());
case MachineOperand::MO_MachineBasicBlock:
case MachineOperand::MO_GlobalAddress:
+ case MachineOperand::MO_ExternalSymbol:
return LowerSymbolOperand(MO, GetSymbolFromOperand(MO));
- case MachineOperand::MO_ExternalSymbol: {
- MCSymbol *Sym = GetSymbolFromOperand(MO);
- Sym->setExternal(true);
- return LowerSymbolOperand(MO, Sym);
- }
case MachineOperand::MO_MCSymbol:
return LowerSymbolOperand(MO, MO.getMCSymbol());
case MachineOperand::MO_JumpTableIndex:
|
Could you point out that does this mean? |
|
The AsmPrinter works on the general principle that everything should be representable in textual assembly: if you produce a .s file and then compile that to a .o file, that should be equivalent to producing the .o file directly. To ensure that this works, the AsmPrinter can't modify MC datastructures directly; it has to make requests through the MCStreamer API. For example, if you want change symbol attributes, you have to go through MCStreamer::emitSymbolAttribute(). If you're emitting an object file directly, this will just modify the relevant datastructure... but if you're emitting assembly, it knows what directive to emit that has the equivalent effect. |
I see! thank you for pointing this out, this makes sense. Would it be ok if I make the change through MCStreamer API, since this comment seems to say that |
|
I'm not sure what the actual goal of the original patch is. If a symbol is not defined, it defaults to being external. If a symbol is defined, we emit the directives for the definition at the point of the definition. So I'm not sure what effect you're hoping for, besides breaking Julia's hack. |
The goal was definitely not to break Julia's hack 🙊. We (Mojo) were trying to do a MC level linking so that we break llvm module into multiple submodules to compile and codegen in parallel (technically into *.o files with symbol linkage type change), but instead of making them into one Granted, it was a bit hacky on our side too (I'm definitely not a MC expert and was hacking my way through our un-conventional use of the backend here). There is probably also some nuance on ORC JIT side why without this change, it doesn't work. I was mostly looking at Hope this provides some context. I'm going to give a talk about the details on what we did next month at EuroLLVM. Would definitely love to hears suggestions/advices on how to make this change better and more disciplined, and I'm also in the process of preparing upstream PRs to show how our parallel strategy looks like). Both PRs are WIP: An early talk about the general idea: https://youtu.be/yuSBEXkjfEA?si=HjgjkxJ9hLfnSvBj&t=813 |
|
Ah, here is more info on the error from ORC JIT. Without this change, if we try to execute generated object file through ORC JIT, it complains about a duplicated external symbol error |
|
@efriedma-quic can you please tell us how to add test for such functionality ? |
Because ARM64 marks the symbol external, or because of some inherent difference in asm between x86 and arm64, or because you have some other change that means you don't trip over it? I still don't really understand why you think the right solution is to override the linkage of a symbol which already has a definition with an explicitly specified linkage.
That sounds like a bug in ORC JIT: JIT linking should have the same semantics as regular linking, and an invalid input object file shouldn't cause an assertion failure. I think we should just merge the straight revert for now. Then we explore the issues some more, and see if there's some better solution for whatever issue your "MCLinker" code is tripping over. |
If you want to check asm output, just run |
Ok, we will try to work on our side to find a different way to fix our issue. Though, this |
efriedma-quic
deleted the
revert-108880-weiweic/mark-x86-externalsymbol-mcsymbol-external
branch
|
/cherry-pick cd6e959 |
|
/pull-request #133348 |
…3291) Reverts llvm#108880 . The patch has no regression test, no description of why the fix is necessary, and the code is modifying MC datastructures in a way that's forbidden in the AsmPrinter. Fixes llvm#132055. (cherry picked from commit cd6e959)
Reverts #108880 .
The patch has no regression test, no description of why the fix is necessary, and the code is modifying MC datastructures in a way that's forbidden in the AsmPrinter. And the author didn't respond to a post-commit review comment requesting a regression test.
Fixes #132055.