[WebAssembly] Preserve ordering of global symbols

[sbc100]

Summary

This change restores the behaviour that global indexes
are assigned in object file order. This was accidentally
changed in https://reviews.llvm.org/D40859.

[ncw]

Looks good. Is the ordering mainly for the stability of the test output, or is it expected to have a tiny improvement to code locality at runtime?

There might be a couple of other places where we could do the same change. For example calculateImports has exactly the same iteration pattern, where the imports are ordered according to the first-importing file, rather than ordered by the file that actually uses the import. Harder to see how to fix that one though. I wouldn't have thought it's worth trying.

[ruiu]

LGTM

wasm/Writer.cpp

617–618 ↗

(On Diff #126224)

Could you invert the boolean expression so that we can still do early continue (to keep the indentation shallower)?

[sbc100]

In D41038#950727, @ncw wrote:

Looks good. Is the ordering mainly for the stability of the test output, or is it expected to have a tiny improvement to code locality at runtime?

There might be a couple of other places where we could do the same change. For example calculateImports has exactly the same iteration pattern, where the imports are ordered according to the first-importing file, rather than ordered by the file that actually uses the import. Harder to see how to fix that one though. I wouldn't have thought it's worth trying.

I think an aesthetic things. It makes sense for function and global definitions to appear in declaration order.

For import, I think it makes sense for them to appear in the order of the first object the contains the import. I can't imagine a object that imports something but doesn't use it.. at least clang shouldn't produce such a thing.

wasm/Writer.cpp

617–618 ↗

(On Diff #126224)

I personally found it more readable this way around. But sure, will change it back

[sbc100]

• feedback

lld/trunk/test/wasm/data-layout.ll

 ; CHECK-NEXT:         Mutable:         true
 ; CHECK-NEXT:         InitExpr:        
 ; CHECK-NEXT:           Opcode:          I32_CONST
 ; CHECK-NEXT:           Value:           66608
 ; CHECK-NEXT:       - Type:            I32
 ; CHECK-NEXT:         Mutable:         false
 ; CHECK-NEXT:         InitExpr:
 ; CHECK-NEXT:           Opcode:          I32_CONST
-; CHECK-NEXT:           Value:           1052
+; CHECK-NEXT:           Value:           1024
 ; CHECK-NEXT:       - Type:            I32
 ; CHECK-NEXT:         Mutable:         false
 ; CHECK-NEXT:         InitExpr:
 ; CHECK-NEXT:           Opcode:          I32_CONST
-; CHECK-NEXT:           Value:           1024
+; CHECK-NEXT:           Value:           1040
 ; CHECK-NEXT:       - Type:            I32
 ; CHECK-NEXT:         Mutable:         false
 ; CHECK-NEXT:         InitExpr:
 ; CHECK-NEXT:           Opcode:          I32_CONST
-; CHECK-NEXT:           Value:           1040
+; CHECK-NEXT:           Value:           1048
 ; CHECK-NEXT:       - Type:            I32
 ; CHECK-NEXT:         Mutable:         false
 ; CHECK-NEXT:         InitExpr:
 ; CHECK-NEXT:           Opcode:          I32_CONST
-; CHECK-NEXT:           Value:           1048
+; CHECK-NEXT:           Value:           1052
 
 ; CHECK:       - Type:            DATA
 ; CHECK-NEXT:     Relocations:
 ; CHECK-NEXT:       - Type:            R_WEBASSEMBLY_MEMORY_ADDR_I32
-; CHECK-NEXT:         Index:           1
+; CHECK-NEXT:         Index:           4
 ; CHECK-NEXT:         Offset:          0x0000001F
 ; CHECK-NEXT:     Segments:
 ; CHECK-NEXT:       - SectionOffset:   7
 ; CHECK-NEXT:         MemoryIndex:     0
 ; CHECK-NEXT:         Offset:
 ; CHECK-NEXT:           Opcode:          I32_CONST
 ; CHECK-NEXT:           Value:           1024
 ; CHECK-NEXT:         Content:         0100000000000000000000000000000003000000000000001C040000

lld/trunk/wasm/Writer.cpp

void Writer::assignSymbolIndexes() {
   }
 
   if (Config->EmitRelocs)
     DefinedGlobals.reserve(Symtab->getSymbols().size());
 
   for (ObjFile *File : Symtab->ObjectFiles) {
     DEBUG(dbgs() << "assignSymbolIndexes: " << File->getName() << "\n");
     for (Symbol *Sym : File->getSymbols()) {
-      if (Sym->hasOutputIndex() || !Sym->isDefined())
+      // Assign indexes for symbols defined with this file.
+      if (!Sym->isDefined() || File != Sym->getFile())
         continue;
-
       if (Sym->isFunction()) {
-        if (Sym->getFile() && isa<ObjFile>(Sym->getFile())) {
-          auto *Obj = cast<ObjFile>(Sym->getFile());
-          Sym->setOutputIndex(Obj->FunctionIndexOffset +
-                              Sym->getFunctionIndex());
-        }
       } else if (Config->EmitRelocs) {
         DefinedGlobals.emplace_back(Sym);
         Sym->setOutputIndex(GlobalIndex++);
       }
     }
   }
 }