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 Feb 4 2022, 6:59 AM.

Details

Summary

Reland of 00bf4755.

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.Feb 4 2022, 6:59 AM
pmatos requested review of this revision.Feb 4 2022, 6:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 4 2022, 6:59 AM
pmatos added inline comments.Feb 4 2022, 7:13 AM
llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
185–194

To be honest, we shouldn't be using Subtarget or TLI here at all since using these means we are inside a Function. But we aren't, what we do is set Subtarget from the last Function we saw. Which means that it will always be assigned to the Subtarget from the last seen Function. This might be ok in the Wasm backend but not in general and breaks when no function is defined. However, as far as I can tell from doing some checks whenever we call this _and_ Subtarget is null, we don't need to call computeLegalValueVTs therefore I added this check.

386

Added a comment here so we don't forget why this is necessary. Removing this call will cause hidden-decl.ll to fail.

llvm/test/CodeGen/WebAssembly/hidden-decl.ll
1 ↗(On Diff #405955)

This test is the test that I extracted from the emscripten failure you referred to in D118122.

sbc100 added a comment.Feb 4 2022, 7:34 AM

lgtm % comments

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

Just emitExternalDecls(*MMI->getModule()) maybe?

llvm/test/CodeGen/WebAssembly/hidden-decl.ll
1 ↗(On Diff #405955)

IIUC that "hidden" thing is no actually important there... isn't the important part that there are no functions declared here?

17 ↗(On Diff #405955)

Can we make this a little simpler and more obvious what its testing?

Maybe just a function foo and some data foo_ptr which stores a pointer to that function? (I think you can remove all the function args, the 'hidden` keywords and the #0 at the end).

pmatos added inline comments.Feb 4 2022, 8:10 AM
llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
555

Sure!

llvm/test/CodeGen/WebAssembly/hidden-decl.ll
1 ↗(On Diff #405955)

Correct, hidden is not necessary, just part of the file that I extracted.

17 ↗(On Diff #405955)

Sure, I will do the change now.

pmatos updated this revision to Diff 405986.Feb 4 2022, 8:48 AM

@sbc100 addresses your concerns.

sbc100 accepted this revision.Feb 4 2022, 9:06 AM

Are you able to verify this against emscripten? Perhaps run the core2 test suite there? If not let me know and I can give it a go.

llvm/test/CodeGen/WebAssembly/only-data.ll
3

How about adding a comment about why we do this? "Verify that types are output for external symbols, even in the absence of any defined functions".

15

Remove trailing newline

This revision is now accepted and ready to land.Feb 4 2022, 9:06 AM

Are you able to verify this against emscripten? Perhaps run the core2 test suite there? If not let me know and I can give it a go.

Yes, the ubsan tests just finished and passing. I was going to give a go on the emscripten as well, but I am having some issues with the testsuites. For some reason running tests/runner shows no testsuites available. I will contact you in pvt.

pmatos updated this revision to Diff 406026.Feb 4 2022, 10:10 AM

Address last set of comments.

Are you able to verify this against emscripten? Perhaps run the core2 test suite there? If not let me know and I can give it a go.

Yes, the ubsan tests just finished and passing. I was going to give a go on the emscripten as well, but I am having some issues with the testsuites. For some reason running tests/runner shows no testsuites available. I will contact you in pvt.

Ah sorry, found my mistake. I am running the core2 tests of emscripten.

@sbc100 might have an incorrect version of node as I keep getting /usr/bin/node: invalid value for --unhandled-rejections. Other than that there are no failures.
Maybe you can give it a go on your side just to be certain before landing?

@sbc100 might have an incorrect version of node as I keep getting /usr/bin/node: invalid value for --unhandled-rejections. Other than that there are no failures.
Maybe you can give it a go on your side just to be certain before landing?

Yup, sounds unrelated to this change, ... is that just a warning or do one or more tests fail? What version is /usr/bin/node (we should probably detect this incompatibility)?

@sbc100 might have an incorrect version of node as I keep getting /usr/bin/node: invalid value for --unhandled-rejections. Other than that there are no failures.
Maybe you can give it a go on your side just to be certain before landing?

Yup, sounds unrelated to this change, ... is that just a warning or do one or more tests fail? What version is /usr/bin/node (we should probably detect this incompatibility)?

Only the fails related to the warning:

test_openjpeg (test_core.core2) ... FAIL
-- begin program output --
/usr/bin/node: invalid value for --unhandled-rejections
-- end program output --
node --version
v12.22.5

It's the ubuntu 21.10 version. What's the minimum version for emscripten?

@sbc100 might have an incorrect version of node as I keep getting /usr/bin/node: invalid value for --unhandled-rejections. Other than that there are no failures.
Maybe you can give it a go on your side just to be certain before landing?

Yup, sounds unrelated to this change, ... is that just a warning or do one or more tests fail? What version is /usr/bin/node (we should probably detect this incompatibility)?

Only the fails related to the warning:

test_openjpeg (test_core.core2) ... FAIL
-- begin program output --
/usr/bin/node: invalid value for --unhandled-rejections
-- end program output --
node --version
v12.22.5

It's the ubuntu 21.10 version. What's the minimum version for emscripten?

Version 16.x of node is giving better results but still have tests failing with:

test_unaligned (test_core.core2) ... skipped 'LLVM marks the reads of s as fully aligned, making this test invalid'
-- begin program output --
/usr/bin/node: bad option: --experimental-wasm-bigint
-- end program output --

I guess I need 17.x or so...

sbc100 accepted this revision.Feb 4 2022, 12:09 PM

Looks good to me! don't worry about configuring your node correctly (if you want one that will certainly work use the emsdk version).

pmatos added a comment.Feb 4 2022, 1:00 PM

Looks good to me! don't worry about configuring your node correctly (if you want one that will certainly work use the emsdk version).

OK - thanks. Lets get this landed. Thanks for your time.

This revision was landed with ongoing or failed builds.Feb 4 2022, 1:02 PM
This revision was automatically updated to reflect the committed changes.