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

Mon Feb 15 10:41:05 PST 2021

dexonsmith added a comment.

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.

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

Repository:
  rL LLVM

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

https://reviews.llvm.org/D41669