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

[weiwei chen via llvm-commits]

weiweichen wrote:

> This solution appears to potentially mask an underlying issue.
> 
> Can you state the motivation behind the change?
> 
> The first few paragraphs only describe "what" the scenario is, but doesn't really state your end goal, and how this helps your MC Linker effort.
> 
> > In X86MCInstLower::LowerMachineOperand, a new MCSymbol can be created in GetSymbolFromOperand(MO) where MO.getType() is MachineOperand::MO_ExternalSymbol
> 

So for MC Linker, the main difference from normal codegen flow is that we are trying to AsmPrint functions that are initially in one llvm module, then being split into submodules for parallel codegen, and now use one AsmPrinter to generate the output. 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`. If 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. This is not what we want for external symbols. Using `AsmPrinter.OutContext` helps, since it is shared between, 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.

Let me add this to the PR description as well!

> It would be beneficial to broaden the review process, especially when the reviewer selected is closely associated and neither of you has prior experience with this specific code area. (#108880 landed quickly within four hours. Considering the reviewer's close association, a wider review might offer additional perspectives.)

Totally agree. The PR  is "draft" now so that I can put out the code to discuss with @npanchen about the solution, but not incur too much noises to others. Once it is "Ready for review", I'll add folks who have given us feedbacks on the previous PR+issue as reviewers. 

(sorry for being pedantic here) But to my defense, #108880 took 4 days to land instead of 4 hours (it was 4 hours after approval). The PR notified core maintainers like `llvm/pr-subscribers-backend-x86` and requested review from folks who are not close to our org based on GitHub suggestion, but I didn't get any comments from anyone other than a post merge one about adding a test. Sorry that I probably missed/forgot that comment. Fully agree that I should have added a test to #108880 to indicate our intention for the change+verify the change. On the other hand, there are numerous x86 backend tests existing and none is broken by the change is a way of testing, no?



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