[PATCH] D48783: [ThinLTO] Use std::map for import lists to get determistic imports files

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D48783#1148452, @steven_wu wrote:

> In https://reviews.llvm.org/D48783#1148427, @tejohnson wrote:
>
> > In https://reviews.llvm.org/D48783#1148412, @steven_wu wrote:
> >
> > > Does the iteration order of those map matters other than changing the hash? Will it change symbol resolution or importing different function?
> >
> >
> > No effect on importing/symbol resolution etc. In fact, this is not an input to the compiler at all, it is used by the build system. The change in order will just make it look to the build system like it has changed when it hasn't.
>
>
> I understand. But if the order is used by the build system, it can also affect the output binary.


I'm not sure I understand how - additional ThinLTO backend processes could be unnecessarily redone, but they would have the same inputs and produce the same object file as before.

> If the only goal is to keep the thin link function import map constant when the input doesn't change, std::map is fine. Can you add a test case for this?

I'll add a test that emits an imports file with multiple lines and check for them in order - it wouldn't necessarily fail without this change (we could get lucky), but it might catch any regression in this ordering.

> 
> 
>>> The other option is MapVector, which preserve the insertion order, which might be faster than std::map.
>> 
>> The trickiness is that currently it may not be inserted in a deterministic order, because we use StringMap elsewhere in the thin link. So std::map is the most straightforward way to get the desired behavior.
> 
> This is unfortunate. I can't think of anything can be wrong because of that now but might need to revisit.




Repository:
  rL LLVM

https://reviews.llvm.org/D48783