[all-commits] [llvm/llvm-project] 1f72fa: [X86Backend][M68KBackend] Make Ctx in X86MCInstLow...

[weiwei chen via All-commits]

  Branch: refs/heads/main
  Home:   https://github.com/llvm/llvm-project
  Commit: 1f72fa29ecb4b283f449c8bf931fcaf0fa1069ee
      https://github.com/llvm/llvm-project/commit/1f72fa29ecb4b283f449c8bf931fcaf0fa1069ee
  Author: weiwei chen <weiwei.chen at modular.com>
  Date:   2025-04-04 (Fri, 04 Apr 2025)

  Changed paths:
    M llvm/lib/Target/M68k/M68kMCInstLower.cpp
    M llvm/lib/Target/X86/X86MCInstLower.cpp
    M llvm/unittests/CodeGen/CMakeLists.txt
    A llvm/unittests/CodeGen/X86MCInstLowerTest.cpp

  Log Message:
  -----------
  [X86Backend][M68KBackend] Make Ctx in X86MCInstLower (M68KInstLower) the same as AsmPrinter.OutContext  (#133352)

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



Looking at other backends, for example `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

It creates/gets the MCSymbol from `AsmPrinter.OutContext` instead of
from `Ctx`. Moreover, `Ctx` for `AArch64MCLower` is the same as
`AsmPrinter.OutContext`.
https://github.com/llvm/llvm-project/blob/8e7d6baf0e013408be932758b4a5334c14a34086/llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp#L100.
This applies to almost all the other backends except X86 and M68k.

```
$git grep "MCInstLowering("
lib/Target/AArch64/AArch64AsmPrinter.cpp:100:      : AsmPrinter(TM, std::move(Streamer)), MCInstLowering(OutContext, *this),
lib/Target/AMDGPU/AMDGPUMCInstLower.cpp:223:  AMDGPUMCInstLower MCInstLowering(OutContext, STI, *this);
lib/Target/AMDGPU/AMDGPUMCInstLower.cpp:257:  AMDGPUMCInstLower MCInstLowering(OutContext, STI, *this);
lib/Target/AMDGPU/R600MCInstLower.cpp:52:  R600MCInstLower MCInstLowering(OutContext, STI, *this);
lib/Target/ARC/ARCAsmPrinter.cpp:41:        MCInstLowering(&OutContext, *this) {}
lib/Target/AVR/AVRAsmPrinter.cpp:196:  AVRMCInstLower MCInstLowering(OutContext, *this);
lib/Target/BPF/BPFAsmPrinter.cpp:144:    BPFMCInstLower MCInstLowering(OutContext, *this);
lib/Target/CSKY/CSKYAsmPrinter.cpp:41:    : AsmPrinter(TM, std::move(Streamer)), MCInstLowering(OutContext, *this) {}
lib/Target/Lanai/LanaiAsmPrinter.cpp:147:  LanaiMCInstLower MCInstLowering(OutContext, *this);
lib/Target/Lanai/LanaiAsmPrinter.cpp:184:  LanaiMCInstLower MCInstLowering(OutContext, *this);
lib/Target/MSP430/MSP430AsmPrinter.cpp:149:  MSP430MCInstLower MCInstLowering(OutContext, *this);
lib/Target/Mips/MipsAsmPrinter.h:126:      : AsmPrinter(TM, std::move(Streamer)), MCInstLowering(*this) {}
lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:695:    WebAssemblyMCInstLower MCInstLowering(OutContext, *this);
lib/Target/X86/X86MCInstLower.cpp:2200:  X86MCInstLower MCInstLowering(*MF, *this);
```

This patch makes `X86MCInstLower` and `M68KInstLower` to have their
`Ctx` from `AsmPrinter.OutContext` instead of getting it from
`MF.getContext()` to be consistent with all the other backends.

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 codegen pipeline till the end of code emission (AsmPrint),
`AsmPrinter.OutContext` is the same as MachineFunction's MCContext, so
this change is an NFC.

----

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.



To unsubscribe from these emails, change your notification settings at https://github.com/llvm/llvm-project/settings/notifications