[llvm] [X86Backend] Use GetExternalSymbolSymbol for MO_ExternalSymbol. (PR #133352)

[via llvm-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-backend-x86

Author: weiwei chen (weiweichen)

<details>
<summary>Changes</summary>

In `X86MCInstLower::LowerMachineOperand`, a new `MCSymbol` can be created in `GetSymbolFromOperand(MO)` where `MO.getType()` is `MachineOperand::MO_ExternalSymbol` 
```
  case MachineOperand::MO_ExternalSymbol:
    return LowerSymbolOperand(MO, GetSymbolFromOperand(MO));
```
at https://github.com/llvm/llvm-project/blob/725a7b664b92cd2e884806de5a08900b43d43cce/llvm/lib/Target/X86/X86MCInstLower.cpp#L196

However, this newly created symbol will not be marked properly with its `IsExternal` field since `Ctx.getOrCreateSymbol(Name)` doesn't know if the newly created `MCSymbol` is for `MachineOperand::MO_ExternalSymbol`.

Use `AsmPrinter.GetExternalSymbolSymbol(Name)` instead for this case so that we actually create a proper MCSymbol that is external. This is similar to what `Arch64MCInstLower` is doing for handling `MC_ExternalSymbol`

https://github.com/llvm/llvm-project/blob/14c36db16fc090ef494ff6d8207562c414b40e30/llvm/lib/Target/AArch64/AArch64MCInstLower.cpp#L366-L367

https://github.com/llvm/llvm-project/blob/14c36db16fc090ef494ff6d8207562c414b40e30/llvm/lib/Target/AArch64/AArch64MCInstLower.cpp#L145-L148

Note that the critical point here is that `MO_ExternalSymbol` is not created/get from `AsmPrinter.OutContext` instead of from `Ctx`.

This fixes an error while running the generated code in ORC JIT for our use case with [MCLinker](https://youtu.be/yuSBEXkjfEA?si=HjgjkxJ9hLfnSvBj&t=813) (see more details below):
https://github.com/llvm/llvm-project/pull/133291#issuecomment-2759200983

We (Mojo) are 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 archive all of them into one `.a` file, we want to fix the symbol linkage type and still produce one *.o file. The parallel codegen pipeline generates the codegen data structures in their own `MCContext` (which is `Ctx` here). So if function `f` and `g` got split into different submodules, they will have different `Ctx`. And when we try to create an external symbol with the same name for each of them with `Ctx.getOrCreate(SymName)`, we will get two different `MCSymbol*` because `f` and `g`'s `MCContext` are different and they can't see each other. This is unfortunately not what we want for external symbols. Using `AsmPrinter.OutContext` helps, since it is shared, if we try to get or create the `MCSymbol` there, we'll be able to deduplicate.

I think since normal use case (probably anything other than our un-conventional case) only handles one llvm module all the way through in the whole codegen pipeline, there is no need to deduplicate, and this change is probably just an NFC.

---
Full diff: https://github.com/llvm/llvm-project/pull/133352.diff


1 Files Affected:

- (modified) llvm/lib/Target/X86/X86MCInstLower.cpp (+9-2) 


``````````diff
diff --git a/llvm/lib/Target/X86/X86MCInstLower.cpp b/llvm/lib/Target/X86/X86MCInstLower.cpp
index 3f6cd55618666..c93a2879a057f 100644
--- a/llvm/lib/Target/X86/X86MCInstLower.cpp
+++ b/llvm/lib/Target/X86/X86MCInstLower.cpp
@@ -192,8 +192,15 @@ MCSymbol *X86MCInstLower::GetSymbolFromOperand(const MachineOperand &MO) const {
   }
 
   Name += Suffix;
-  if (!Sym)
-    Sym = Ctx.getOrCreateSymbol(Name);
+  if (!Sym) {
+    // If new MCSymbol needs to be created for
+    // MachineOperand::MO_ExternalSymbol, create it as a symbol
+    // in AsmPrinter's OutContext.
+    if (MO.isSymbol())
+      Sym = AsmPrinter.OutContext.getOrCreateSymbol(Name);
+    else
+      Sym = Ctx.getOrCreateSymbol(Name);
+  }
 
   // If the target flags on the operand changes the name of the symbol, do that
   // before we return the symbol.

``````````

</details>


https://github.com/llvm/llvm-project/pull/133352