I guess you have to do this to make it NFC, since it looks like we were doing this in the ModuleLinker.
Interesting...I didn't realize or had forgotten that the ModuleLinker would do this - so essentially we always import an alias if its aliasee is linkonce_odr and marked for import? That seems like something we want to get rid of (can be in a follow-up change so this remains NFC). We've tried to get to the point where we only import values decided on by the thin link. This is presumably a holdover from the old scheme of always importing linkonce_odr references...

Yes, and also needed in order to be NFC. Without this we would import @linkonceODRfuncLinkonceAlias in test/ThinLTO/X86/alias_import.ll because the logic for whether to put globals in GlobalsToImport differed from that in FunctionImportGlobalProcessing::doImportAsDefinition and we would normally only import globals that satisfied both (this is because we were querying the latter on LinkModules.cpp:394 in the old code and querying the former on line 277).

But naturally that raises the question of why the change on line FunctionImport.cpp:724 is needed at all. The answer is that we only call shouldLinkFromSource if the destination defines a GV with the same name, so if the destination did not define the GV we would only test FunctionImportGlobalProcessing::doImportAsDefinition.

In short: ModuleLinker is complicated, and it's definitely a Good Thing that we're moving away from it.

LGTM but please remove the NFC from the description, due to the isInterposable change and the comdat handling difference noted on IRC. I did build SPEC with this change and it isn't causing any failures (there is a failure to link soplex due to hidden symbols, but it is still there without this patch and likely due to D28978).