[PATCH] D35148: Use DenseMap instead std::map for GVSummaryMapTy.

[Dehao Chen via llvm-commits]

On Mon, Jul 10, 2017 at 11:47 AM, Mehdi AMINI <joker.eph at gmail.com> wrote:

>
>
> 2017-07-10 11:40 GMT-07:00 Teresa Johnson <tejohnson at google.com>:
>
>>
>>
>> On Mon, Jul 10, 2017 at 11:36 AM, Dehao Chen via Phabricator <
>> reviews at reviews.llvm.org> wrote:
>>
>>> danielcdh added a comment.
>>>
>>> How should we proceed with this patch?
>>>
>>> There are indeed some code that iterates through this type, e.g.:
>>>
>>
>> Right, that's what I was referring to here: "The only place I see an
>> iteration is when we are computing imports for a module, so I suppose that
>> could affect the debug output ordering, but does that matter (if iteration
>> order is even non-deterministic for DenseMap, and from what I can tell from
>> the LLVM doc linked above it isn't)."
>>
>> I don't think this is an issue, and from Mehdi's response it sounds like
>> he agrees.
>>
>> I think the patch is good to go. Mehdi - do you concur?
>>
>
> I'm not totally sure: for example we won't call `computeImportForFunction`
> in the same order.
> Right now I don't see how it should matter though.
>
> However, as long as the iteration order is deterministic, it does not
> matter if it is undefined: we still have full reproducibility.
> It would be an issue if we have multiple-threads that lock and insert to
> this map though, as it would make the iteration order non reproducible. If
> it is not case I think we're good!
>

The map is created/updated by llvm::ComputeCrossModuleImportForModule
and ModuleSummaryIndex::collectDefinedGVSummariesPerModule, neither of
which seem to be called concurrently.

Thanks for the reviews! I'll commit the patch.

Dehao


>
> Hope it makes sense.
>
> --
> Mehdi
>
>
>
>
>>
>>
>>>   // Populate the worklist with the import for the functions in the
>>> current
>>>   // module
>>>   for (auto &GVSummary : DefinedGVSummaries) {
>>>     if (!Index.isGlobalValueLive(GVSummary.second)) {
>>>       DEBUG(dbgs() << "Ignores Dead GUID: " << GVSummary.first << "\n");
>>>       continue;
>>>     }
>>>     auto *Summary = GVSummary.second;
>>>     if (auto *AS = dyn_cast<AliasSummary>(Summary))
>>>       Summary = &AS->getAliasee();
>>>     auto *FuncSummary = dyn_cast<FunctionSummary>(Summary);
>>>     if (!FuncSummary)
>>>       // Skip import for global variables
>>>       continue;
>>>     DEBUG(dbgs() << "Initalize import for " << GVSummary.first << "\n");
>>>     computeImportForFunction(*FuncSummary, Index, ImportInstrLimit,
>>>                              DefinedGVSummaries, Worklist, ImportList,
>>>                              ExportLists);
>>>   }
>>>
>>>
>>> https://reviews.llvm.org/D35148
>>>
>>>
>>>
>>>
>>
>>
>> --
>> Teresa Johnson |  Software Engineer |  tejohnson at google.com |
>> 408-460-2413 <(408)%20460-2413>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170710/6d9217b7/attachment.html>