In D95425#2810027, @DavidSpickett wrote:

Also affecting riscv-v (riscvv?) so I've reverted the change.

https://lab.llvm.org/buildbot/#/builders/67/builds/3087

Hopefully we are close to landing this. :)

Fix details requested by @tlively in latest comemnts.

@tlively thanks for the comments. I am currently taking some time off but I will address your concerns upon my return.

@deadalnix @tlively @sbc100 anything missing here to get this landed?

Rebase and update minor details and fix a few lint warnings...
Non-functional changes only.

Making the linter happy...

• Simplifies some of code in the middle-end.
• Removes setComdat and adds setWeak

Removed extraneous changes to previous patches

Fix broken merge of table ins reordering commit

Fix patch set... again!

Fix patch submitted to phab...

Rebase this on D101608 from @wingo and clear table slot after call

Merged by @tlively as https://github.com/llvm/llvm-project/commit/cd460c4d11eeeca485353ad6a1961b543980b664
In the meantime I got committers rights. Thanks.

In D101656#2731010, @sbc100 wrote:

I don't think there are any uses of these instructions in emscripten and I doubt there are any emscripten users using the assembly format.

@sunfishcode do you know of anyone using these assembly instructions?

In D101656#2730556, @dschuff wrote:

I think all of emscripten's use of the table is still via the JS API (e.g. src/runtime_functions.js) so this probably won't break emscripten. It does seem likely that nobody is using these instructions via assembly.
Is this the only change needed? Do we have no tests of these asm instructions anywhere in LLVM at all?

A quick note - apparently phab felt that since I accepted the revision, it's now ready to land. Probably obvious but I accepted expecting @tlively comments to be addressed first. :)

This looks good to me - I will rebase D95425 on this.

Make TABLE_SET node polymorphic. Deal with types through the Pats. One for funcref and one for externref.

Fix else if and VT type as suggested by @wingo.

There are still two things missing:

1. Clear the table entry after the call_indirect to avoid hidden GC roots as mentioned by @wingo
2. Understand how to handle a ret %extern undef - comment by @tlively

Add missing newlines at the end of test files

Take care of some syntax linting/style comments

Use PointerType to avoid defining types for externref and funcref.

Apparently getTypeForEVT in LowerArguments is only necessary to call the getABIAlignmentForCallingConv. I was thinking that since reference types really are pointers to different address spaces we could return their TypeForEVT as being int32. That will then return as the alignment for reference types the default alignment for int32. This should help with avoiding us touching middle end code. I will experiment in this direction.

In D95425#2699048, @tlively wrote:

I'm still not comfortable approving this patch with changes to llvm-c/, Bitcode/, or IR/. If those changes can't be backed out, this will require an RFC on llvm-dev, but I would be surprised if that were necessary.

Are there any further comments to this patch before landing?

Fix table name for funcref calls

Clarify many of the issues requested in previous review comments.

In D95425#2646638, @tlively wrote:

In D95425#2643765, @pmatos wrote:

In D95425#2643730, @lebedev.ri wrote:

Once again, are the changes to the LLVM IR (LLVMTypeKind & friends) actually needed?
I'm not seeing any usages of that in the tests.

Hi, I was going refer to those in a separate comment. My apologies for the delay.
Those were required to get this to work due to a call in SelectionDAGBuilder.cpp (current like 9966) to VT.getTypeForEVT. This requires me to create a type for the EVT externref and funcref. This forced the change to ValueTypes.cpp. Once you create the Type for externref and funcref the other changes come along either by necessity or to avoid warnings that certain cases were not handled in switches.

It would make sense for VT.getTypeForEVT to return the appropriate address space pointer types that we are lowering to externref and funcref, right?

In D95425#2643730, @lebedev.ri wrote:

Once again, are the changes to the LLVM IR (LLVMTypeKind & friends) actually needed?
I'm not seeing any usages of that in the tests.

Reformat patch with clang-format-diff.py

Small fixes:

• Set Align(1) for reference types
• Remove unused node
• Make webassembly target description string dependent on feature string (fixes clang tests)

In D95425#2632834, @lebedev.ri wrote:

Why are IR-level changes needed if these are generated during lowering?
Will this play well with typeless pointers?

Remove commented code that shouldn't be there in the first place.

A few data points on the patch. Externrefs live in address space 1 and funcrefs in address space 3. Externref globals live in address space 2 and funcref globals live in address space 4.

Complete patch ready for review.

Attempt to specify externref as a pointer type with a fixed size of 32, instead of opaque.

Fix getLegalType for simple MVTs

This is very, very WIP patch implementing LLVM IR support for global.get/set.

In D91428#2413292, @pmatos wrote:

Thanks, @arichardson and @jrtc27 for your comments.
I am definitely surprised to find that if you explicitly mark the call with the address space, this patch is not required. At first look, this RFC is not required any more but I need sometime to investigate further. If no changes are necessary, this is indeed good news.

Thanks, @arichardson and @jrtc27 for your comments.
I am definitely surprised to find that if you explicitly mark the call with the address space, this patch is not required. At first look, this RFC is not required any more but I need sometime to investigate further. If no changes are necessary, this is indeed good news.

The RFC has now been sent to the mailing list: http://lists.llvm.org/pipermail/llvm-dev/2020-November/146723.html

Fix type check.

Ensure the program address spaces vector doesn't contain duplicates.

This is WIP - there a still a few test failures but I am happy to start getting comments on this.

@sbc100 if it's ok for you, feel free to merge, thanks.

Align with local indent style

Remove unused operand type.

Is this good to merge?

Regarding linter clang-format suggestions - should I reformat the code as suggested or leave it?
Some suggested changes are not necessarily on code I wrote, which is why I ask.

These changes reflect the first set of comments. High on the list is quite a few changes related to setting registers for funcrefs and externrefs. A few things might not be strictly necessary but pave the way for future work.

Thanks for the review - I will submit a new revision.

So the goal here is to add most of the table instructions. This introduces some table relocations, which (in a following patch) we will resolve in the linker.

Further test fixes - missing index

In D88815#2326785, @pmatos wrote:

In D88815#2326737, @sbc100 wrote:

I seeing:

llvm-project/llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:111:11: warning: enumeration value 'FUNCREF' not handled in switch [-Wswitch]
  switch (VT) {

Oh, I was not getting that. It seems to come from https://github.com/llvm/llvm-project/commit/3bba91f64eef15956f589fa446c265a714cc7893 and I hadn't pulled that in yet. If I rebase my patch on this then yes, I can see the warning. i will fix it in this patch. No worries.

Fix warning due to missing FUNCREF in switch

In D88815#2326737, @sbc100 wrote:

I seeing:

llvm-project/llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:111:11: warning: enumeration value 'FUNCREF' not handled in switch [-Wswitch]
  switch (VT) {

Fix a bunch more lld tests to include index for tables

In D88815#2325730, @sbc100 wrote:

Is this ready to land now?

Fix tests to include table index in expected dump

Fix build failure

Move Index to first position in WasmTable struct

Fix review nits

Thanks for the reviews. I will fix the nits mentioned and update the revision.

In D88226#2292764, @sbc100 wrote:

This is already supported like via -mllvm. E.g. wasm-ld -mllvm -debug or wasm-ld -mllvm -debug-only=lld

Update externref patch

Mistakingly created - this should have gone to D66035

In D66035#2181659, @pmatos wrote:

I will be splitting the part enabling the target feature through clang into a separate revision as suggested by @tlively

I will be splitting the part enabling the target feature through clang into a separate revision as suggested by @tlively

Update patch to compile against current master branch

Related work: https://reviews.llvm.org/D81977

There's also this line of work on opaque types that could be potentially interested but seems far from being landed: https://groups.google.com/g/llvm-dev/c/Dw_DYSXGFto/m/OzzK-CkGAwAJ

Please ignore my .gitlab-ci.yml. That's just an internal change that I got uploaded by mistake.
I am looking to see this through and start discussion on this with the goal of landing it.