[PATCH] D41669: Teach ValueMapper to use ODR uniqued types when available

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added a comment.

In D41669#2564112 <https://reviews.llvm.org/D41669#2564112>, @tejohnson wrote:

> In D41669#2563866 <https://reviews.llvm.org/D41669#2563866>, @dexonsmith wrote:
>
>> In D41669#2563798 <https://reviews.llvm.org/D41669#2563798>, @tejohnson wrote:
>>
>>> In D41669#2561267 <https://reviews.llvm.org/D41669#2561267>, @dexonsmith wrote:
>>>
>>>> This is unsound, as it breaks CloneModule: it causes the original/cloned module to reference GlobalValues from the new module (composite types transitively reference global values when a type has "interesting" template parameters). See https://bugs.llvm.org/show_bug.cgi?id=48841 (especially https://bugs.llvm.org/show_bug.cgi?id=48841#c22, where I have a long explanation).
>>>>
>>>> @tejohnson, can you share more context about why this was necessary? How is ThinLTO using the value mapper that this happens?
>>>>
>>>> There should be another way to get this optimization that's sound, but I think it'll involve priming the ValueToValueMap that gets sent into MapValue / MapMetadata.
>>>
>>> I need to go back and look at this some more, unfortunately it's not immediately obvious. It looks like I was making this change for unspecified reasons related to my change to import aliases as a copy of the aliasee, and then found it fixed PR36089. Need to go back and see if the test case in that bug works now or still fails without this change.
>>
>> I'd bet the testcase in that bug will still fail without this change.
>
> Actually - I just tried it and it passes with this change reverted. I guess something else already fixed that issue in a slightly different probably more correct way?

I'm seeing ThinLTO/X86/dicompositetype-unique-alias.ll fail locally. Maybe you didn't have 22a52dfddcefad4f275eb8ad1cc0e200074c2d8a <https://reviews.llvm.org/rG22a52dfddcefad4f275eb8ad1cc0e200074c2d8a> when you tried? (I probably hadn't pushed yet...)

>> Instead of changing ValueMapper, maybe the right fix is similar to the code in `CloneFunctionInto` (which I'm conditionalizing in https://reviews.llvm.org/D96531, expecting to commit soon, just missed the window last week where I could watch the bots): prime the ValueToValueMap with mappings-to-self for any entry points to the metadata graph that shouldn't be cloned / duplicated.
>>
>>> Regarding CloneModule - from my skim of the other bug you are working on it appears that this is causing an issue when invoked while cloning a module when we split ThinLTO bitcode files - is that correct? In my case I was fixing an issue during importing, not during this cloning.
>>
>> Yes, that's correct. Thanks for clarifying this is during importing; it's reasonable at a high level to reuse the ODR logic; the problem is that the value mapper code has more general usecases. (Seems like metadata ownership (especially for `distinct` nodes) needs to be cleaned up in some way... there are a bunch of subtleties that make it hard to reason about.)
>>
>> Is the source module being discarded? If so, the importer could safely use `RF_ReuseAndMutateDistinctMDs` (the new name for `RF_MoveDistinctMDs`).
>
> Yes it's being discarded. And yes it is already using RF_ReuseAndMutateDistinctMDs (set by default on the IRLinker constructor initialization of the ValueMapper). Hmm, now I'm confused. The case I modified in this patch is where this flag is not set.  Aha - going through the debugger for the test case included with this patch shows that while the importer itself uses a ValueMapper with RF_ReuseAndMutateDistinctMDs enabled, the functionality that imports an alias as a copy of the aliasee (replaceAliasWithAliaseee) in fact uses CloneFunction/CloneFunctionInto, so a ValueMapper without that flag. So perhaps your fix in D96531 <https://reviews.llvm.org/D96531> is sufficient and this change can be reverted?

Your SGTM -- replaceAliasWithAliasee DOES use CloneFunction -- but as mentioned above the test is failing for me. I'm confused... maybe my tree is out-of-date, or maybe there's something subtle going on... I'll reply again if I figure it out.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D41669/new/

https://reviews.llvm.org/D41669