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

Mon Feb 15 10:07:30 PST 2021

tejohnson added a comment.

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.

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.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D41669