Remove conflict marker

Draft patch as some tests still fail.

Remove extraneous whitespace.
Make equal CHECK lines use WEBASSEMBLY:

rebase on top of D128282

Incorporate @asb semantic restrictions patch.
Generate test CHECK using update_cc_* script.

Update revision to be specific to externref only.
A different revision will be opened for further work on externref builtins and funcref.

Still need to implement funcrefs support as function arguments.

Rebased the work on D126535. Added support for funcrefs as return values.
Still having problems with funcrefs as function arguments though. So, further work is required.
Not ready for review.

Thanks for the change. LGTM.
I will rebase my further changes on top of this.

This still requires some changes, as there are a few unhandled cases where funcref attribute is not handled properly at the edge.

Moved the attribute handling to the border between clang and llvm.
In the frontend, there's no special handling of address spaces for funcref, but
this is added when converting the values to LLVM types.

A couple of weeks ago @asb pointed out we were missing an intrinsic for ref.is_null, so I got the work started. There's an issue with the polymorphic type for ref types, but I have seen this before and hopefully will get it fixed properly this time.

Test wasm-funcref.c still fails as type attribute funcref semantics are not yet implemented. Not ready to land yet.

lgtm, thanks.

Thanks - all tests pass, landing again.

@nokotan The test line should be something like: llvm-mc -mattr=+reference-types -triple=wasm32-unknown-unknown -filetype=obj -o - < %s | obj2yaml | FileCheck %s
However, I notice that MC/WebAssembly/tables.s is failing as well. Can you pls take a look?

In D120229#3359775, @DavidSpickett wrote:

This caused two test failures on one of our bots: https://lab.llvm.org/buildbot/#/builders/183/builds/3684

For the new test, it's possible you had a wasm-ld on your path and that's why it worked locally.

Please investigate then reland.

In D120229#3358146, @sbc100 wrote:

In D120229#3357544, @nokotan wrote:

I don't have access rights to the llvm repository, arc land failed from my terminal.

@pmatos do you want to take care of landing this? (since its kind of your area). If you are busy can also take care of it.

Thanks for your time working on the patch. It would be great to address @aardappel 's comments. The idea is to merge the function getGlobal with getTable, since they are so similar. I suppose into something like getGlobalOrTable. Add another switch case for wasm::WASM_SYMBOL_TYPE_TABLE to return the appropriate type.

lgtm

Thanks for the patch. In addition to what @sbc100 wrote, some further comments inline.

In D118995#3297737, @sbc100 wrote:

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

In D118995#3297627, @pmatos wrote:

In D118995#3297475, @sbc100 wrote:

In D118995#3297466, @pmatos wrote:

@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?

In D118995#3297475, @sbc100 wrote:

In D118995#3297466, @pmatos wrote:

@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?

In D118995#3297389, @pmatos wrote:

In D118995#3297060, @sbc100 wrote:

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.

Address last set of comments.

In D118995#3297060, @sbc100 wrote:

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.

@sbc100 addresses your concerns.

Attempting to reland this with D118995 after fixing issues found.

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.

In D118122#3287371, @sbc100 wrote:

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.

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

Remove non-NFC change.

Added [NFC] to the title, rebased on main and rechecked that tests are fine.

@sbc100 I have now addressed all your comments, I feel like it is ready for review.

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

@sbc100 Address review comments from D115749.

@sbc100 Thanks for the comments, I have split this into D118121 and D118122. I will now apply the comments from your reviews to the new revisions.

Update the bug summary, no code changes.

Refactored the setting of the Wasm Symbol type into WasmSymbolSetType in WebAssemblyAsmPrinter.cpp and WebAssemblyMCInstLower.cpp. However, this forced us to move the function to WebAssemblyTypeUtilities.h and refactor some other type-related functions from WebAssemblyUtilities to WebAssemblyTypeUtilities.

Full patch for required refactoring and fix.

In D113420#3239319, @ljmf00 wrote:

In D113420#3151549, @pmatos wrote:

After closing D111420, I moved the implementation of all intrinsics to this revision without the need for the polymorphic llvm_anyref_ty type.

@pmatos You likely referenced the wrong diff. I would suggest amending this to avoid confusion in the future :)

In D115749#3248936, @sbc100 wrote:

In D115749#3248640, @pmatos wrote:

@sbc100 I think I understand where this is going wrong. As far as I understand emitExternalDecls should only be emitting .functype, etc directives for external symbols and yet it's doing so for all symbols in OutContext, even those which are not external. I will have to revisit my assumptions and take a look at the code again, since the current solution of running emitExternalDecls in onFinalization means all directives come at the end and that doesn't work because it complains if, for example a reference to bar comes before bars type declaration.

Yes, it makes sense to me that emitExternalDecls only applies to external symbols (those not defined in the module). However, I would have thought that for typechecking reasons (we do basic type checking, unlike other asm backend) that all the external decls always come first.

@sbc100 I think I understand where this is going wrong. As far as I understand emitExternalDecls should only be emitting .functype, etc directives for external symbols and yet it's doing so for all symbols in OutContext, even those which are not external. I will have to revisit my assumptions and take a look at the code again, since the current solution of running emitExternalDecls in onFinalization means all directives come at the end and that doesn't work because it complains if, for example a reference to bar comes before bars type declaration.

Refactored places where type related utility functions are and refactored some of the symbol type creation.

@sbc100 Unfortunately while adding some tests to this including testing private IR globals, which don't emit a .globl for the symbol I found a crash, which I now fixed and an issue with multiple globals in the same compilation unit. For example, two tables don't work because .tabletype is only emitted for the first. emitLinkage is called on each symbol in turn, and emitExternalDecls emits the tabletype for the symbol, however it only runs for the first symbol because emitExternalDecls only actually runs the first time it's called. This is very unfortunately and will need a bit of refactoring. I am working on it.

In D27800#3235066, @efriedma wrote:

Looks like all the review comments have been addressed. LGTM

Ensure that we only call transform on Unevaluated Contexts, avoids the failure of a couple of vla tests.

In D27800#3235066, @efriedma wrote:

Looks like all the review comments have been addressed. LGTM

@efriedma I know it has been a long time, but are you still able to review this?

Fixes https://github.com/llvm/llvm-project/issues/30390

After a long hiatus on this bug, this is still failing on HEAD so lets get it fixed.

Rebased on top of main. All seems good.

Simplified code to emit labels for tables and tabletype directive.

Remove isUndef check from code. I cannot find a codepath that generates an undef Idx at this point. All undef values end up being propogated down to the respective GEP. So simplifying and removing check.