This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Refactor and fix emission of external IR global decls
ClosedPublic

Authored by pmatos on Jan 25 2022, 3:28 AM.

Details

Summary

This patches fixes the visibility and linkage information of symbols
referring to IR globals.

Emission of external declarations is now done in the first execution
of emitConstantPool rather than in emitLinkage (and a few other
places). This is the point where we have already gathered information
about used symbols (by running the MC Lower PrePass) and not yet
started emitting any functions so that any declarations that need to
be emitted are done so at the top of the file before any functions.

This changes the order of a few directives in the final asm file which
required an update to a few tests.

Diff Detail

Event Timeline

pmatos created this revision.Jan 25 2022, 3:28 AM
pmatos requested review of this revision.Jan 25 2022, 3:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 25 2022, 3:28 AM

@sbc100 I addressed your questions regarding this part of the patch in D115749. I think it's ready for your review.

pmatos updated this revision to Diff 404246.Jan 29 2022, 1:43 AM

Move the removal of some lines setting table global type from D118121 to this revision.

llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
197

@sbc100 the lines that were being previously removed in D118121, are now here.

sbc100 accepted this revision.Jan 29 2022, 8:17 AM

lgtm % comments

llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
270

Does this happen? i.e. are there symbols that don't have their type set at this point?

367

I guess this was here to handle the case that emitLinkage was never called? Does that not apply to emitConstantPool too?

In other words, why remove this line?

This revision is now accepted and ready to land.Jan 29 2022, 8:17 AM
pmatos added inline comments.Jan 31 2022, 12:13 AM
llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
270

Yes, just set a breakpoint on the return line (270) and run simd-arith.ll and you'll see a few cases of this.

pmatos added inline comments.Jan 31 2022, 2:38 AM
llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
367

So - in all my tests I couldn't find a reason for this line to exist. And I noticed that whenever it was called, it had generally been called through emitLinkage.

With this patch, emitExternalDecls is not called unless there are functions in the compilation unit. Globals are still emitted properly through emitGlobalVariable though. So this only leaves symbols that are necessary for use in functions, I think. But if those functions exist, they are emitted anyway, so I don't see the need for this.

Note that before this patch, this line was needed because we were emitting global symbol types in emitExternalDecls, and now we have move that to emitGlobalVariable.

This revision was landed with ongoing or failed builds.Jan 31 2022, 2:42 AM
This revision was automatically updated to reflect the committed changes.
bkramer added inline comments.
llvm/test/CodeGen/WebAssembly/table-types.ll
1

This test crashes with -fsanitize=null because Subtarget is a nullptr.

llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:183:56: runtime error: member call on null pointer of type 'llvm::WebAssemblySubtarget'
    #0 0x565025dd6a86 in llvm::WebAssemblyAsmPrinter::emitGlobalVariable(llvm::GlobalVariable const*) llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:183:56
    #1 0x56502684357b in llvm::AsmPrinter::doFinalization(llvm::Module&) llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1756:5
    #2 0x56502b16810e in llvm::FPPassManager::doFinalization(llvm::Module&) llvm/lib/IR/LegacyPassManager.cpp:1499:41
    #3 0x56502b1558ea in runOnModule llvm/lib/IR/LegacyPassManager.cpp:1586:41
    #4 0x56502b1558ea in llvm::legacy::PassManagerImpl::run(llvm::Module&) llvm/lib/IR/LegacyPassManager.cpp:540:44
    #5 0x56502b1683a4 in llvm::legacy::PassManager::run(llvm::Module&) llvm/lib/IR/LegacyPassManager.cpp:1677:14
    #6 0x5650238100ae in compileModule(char**, llvm::LLVMContext&) llvm/tools/llc/llc.cpp:719:8
    #7 0x565023809fb6 in main llvm/tools/llc/llc.cpp:417:22

Probably not the fault of this change though, it just happens when there is no function. Any ideas how to fix it?

pmatos added inline comments.Jan 31 2022, 7:48 AM
llvm/test/CodeGen/WebAssembly/table-types.ll
1

@bkramer Thanks for pointing this out. I am not at work atm. I will take a look as soon as I return. Feel free to revert if its causing some issues elsewhere. I am surprised you found this. I ran regularly all tests. Is there a specific mode which includes running tests with sanitizers?

kda added a subscriber: kda.Jan 31 2022, 11:14 AM

This has broken fast buildbot: https://lab.llvm.org/buildbot/#/builders/5/builds/18257/steps/16/logs/stdio

I will revert in 60 minutes.

kda added inline comments.Jan 31 2022, 11:27 AM
llvm/test/CodeGen/WebAssembly/table-types.ll
1

Sadly this appears to also be breaking the emscripten CI:

https://ci.chromium.org/ui/p/emscripten-releases/builders/try/linux/b8823500584349280721/overview

Sample failure:

test_unistd_unlink (test_core.core0) ... 
wasm-ld: error: symbol type mismatch: __stdio_write
>>> defined as WASM_SYMBOL_TYPE_FUNCTION in /usr/local/google/home/sbc/dev/wasm/emscripten/cache/sysroot/lib/wasm32-emscripten/libc-debug.a(__stdio_write.o)
>>> defined as WASM_SYMBOL_TYPE_DATA in /usr/local/google/home/sbc/dev/wasm/emscripten/cache/sysroot/lib/wasm32-emscripten/libc-debug.a(stderr.o)

I guess we should revert

The emscripten breakage appears to be related to missing function types when function are only references in static data. For example musl's stderr.c: https://github.com/emscripten-core/emscripten/blob/6754be32129e9392a2d1f684a1d02172cda0d640/system/lib/libc/musl/src/stdio/stderr.c#L12-L14

Without this change __stdio_close/seek/write get .function_type directives and with this change they don't. i.e. These lines are missing after this change:

	.functype	__stdio_close (i32) -> (i32)
	.functype	__stdio_write (i32, i32, i32) -> (i32)
	.functype	__stdio_seek (i32, i64, i32) -> (i64)

I'm guessing this is because this translation unit contains no functions.. only data.

sbc100 added a comment.Feb 1 2022, 6:43 AM

I guess that re-enabling the emitExternalDecls in emitEndOfAsmFile would fix the emscripten issue (although that points to a missing test case). Not sure about the other failures reported here.

pmatos added a comment.Feb 3 2022, 6:12 AM

I guess that re-enabling the emitExternalDecls in emitEndOfAsmFile would fix the emscripten issue (although that points to a missing test case). Not sure about the other failures reported here.

I have now added a missing test case and confirmed that the emitExternalsDecls is required in emitEndOfAsmFile.
The null Subtarget issue was supposed to be simple to fix. The Subtarget field is set on a per-function basis and we use that to obtain the TLI. The problem is, if you have no functions defined in the file Subtarget will always be null and never be set. We therefore need to obtain the TLI to pass to computeLegalValuesVT using some other way. I have however been looking at this for over an hour now and for some reason that escapes me the TLI seems to always be linked to a function and not obtainable otherwise. I will need to keep investigating this to be able to push a new patch.

pmatos added a comment.Feb 3 2022, 6:44 AM

I confirmed with Anton K. in discord that indeed it's not possible to obtain a TLI outside a function, because a function can change the subtarget. Apparently ARM allows this with ARM vs Thumb functions being defined in the same file.

That means that we either reimplement computeLegalValueVTs such that it doesn't need to call TLI.getNumRegisters nor TLI.getRegisterType(), or find a way not to call it at all. I am continuing my investigation.

pmatos added a comment.Feb 4 2022, 6:59 AM

Attempting to reland this with D118995 after fixing issues found.