[PATCH] D94202: Preserve the lexical order for global variables during llvm-link merge

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

dexonsmith added a comment.

I have a few nitpick comments inline, but they might be distractions.

Stepping back, this new sort function seems very special purpose, and unlikely to be reused. It's also pretty complicated, with a few different maps... not sure how easy it will be to document / test / etc.

I wonder about doing this instead:

1. Add a sort function to simple_ilist that takes iterators (for the subrange to be sorted). This would be pretty easy to reuse. Maybe something like this would do it:

  template <class Compare>
  void sort(iterator First, iterator Last, Compare comp) {
    simple_ilist Sublist;
    Sublist.splice(Sublist.end(), *this, First, Last);
    Sublist.sort(comp);
    splice(Last, Sublist, Sublist.begin(), Sublist.end());
  }

That'd be pretty easy to add a unit test for as well. Maybe the sublist sort could be a helper extracted from the full list sort (didn't look closely to see if that was easy).

2. Use the generic `Compare` argument for proscribing the new order. You could do something like this:

  DenseMap<GlobalVariable *, int> Order;
  // Build the map table to figure out the desired order of the new
  // globals in the destination module.
  for (GlobalVariable &GV : SrcM->globals()) {
    if (GV.hasAppendingLinkage())
      continue;
    auto NewGV = Mapper.mapValue(GV);
    if (!NewGV)
      continue;
    NewGV = NewGV->stripPointerCasts();
    if (isa<GlobalVariable>(NewGV))
      Order[&NewGV] = Order.size() + 1; // Start from 1.
  }
   
  // Advance the pointer to the first newly added element.
  auto Begin = Globals.begin();
  std::advance(Begin, OldSize);
  
  // Reorder the newly added globals based on the map table.
  Globals.sort(Begin, Globals.end(),
      [&Order](const GlobalVariable &LHS, const GlobalVariable &RHS) {
        return Order.lookup(&LHS) < Order.lookup(&RHS);
      });

Note that I started from 1 because `Order.lookup()` returns 0 for anything not in the map.



================
Comment at: llvm/include/llvm/ADT/simple_ilist.h:324
+                                       iterator First, iterator Last,
+                                       Compare comp) {
+  // Vacuously sorted.
----------------
Is `comp` being used here? I'm not seeing it...


================
Comment at: llvm/include/llvm/ADT/simple_ilist.h:338-339
+  for (auto &G : SourceList) {
+    if (DM.count(&G)) {
+      auto Dest = DM[&G];
+      if (PointerMap.count(Dest)) {
----------------
I think these two dense map lookups can be turned into one with:
```
if (pointer Dest = DM.lookup(&G)) {
```
It'd also be good form to invert the condition to avoid nesting:
```
pointer Dest = DM.lookup(&G);
if (!Dest)
  continue;
```



================
Comment at: llvm/include/llvm/ADT/simple_ilist.h:340-341
+      auto Dest = DM[&G];
+      if (PointerMap.count(Dest)) {
+        auto It = PointerMap[Dest];
+        It++;
----------------
The double-lookup and nesting should be avoided here too, presumably with `.find()`, unless `lookup()` will work with an `iterator` return (I don't think it will but my memory might be wrong).


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

https://reviews.llvm.org/D94202