[WebAssembly] Implementation of global.get/set for reftypes in LLVM IR

[pmatos]

Summary

Reland of 31859f896.

This change implements new DAG notes GLOBAL_GET/GLOBAL_SET, and
lowering methods for load and stores of reference types from IR
globals. Once the lowering creates the new nodes, tablegen pattern
matches those and converts them to Wasm global.get/set.

[pmatos]

This patch would have fixed the problems with AArch64 caused by D95425. However, since that was landed and reverted, this landed: https://github.com/llvm/llvm-project/commit/ac81cb7e6dde9b0890ee1780eae94ab96743569b

This breaks the test llvm/test/CodeGen/WebAssembly/externref-ptrtoint.ll which was expected to fail. I still think that a ptrtoint on an externref value should fail, and yet since ac81cb7e allows ptrtoint on non-integral pointer types, the verifier is letting this test to go through and it crashes LLVM later on during some DAG node analysis.

@tlively Do you think it would be ok to re-add the code removed in ac81cb7e but only error if the pointer is to an opaque non-integral type?

[pmatos]

Adding @reames to subscribers since he's the author of https://github.com/llvm/llvm-project/commit/ac81cb7e6dde9b0890ee1780eae94ab96743569b and might have something to add to the discussion.

[tlively]

In D104797#2836475, @pmatos wrote:

@tlively Do you think it would be ok to re-add the code removed in ac81cb7e but only error if the pointer is to an opaque non-integral type?

Unfortunately that wouldn't be future-proof given the ongoing project to remove type information from pointers. I think the best we can do right now is call report_fatal_error from the backend whenever a reference type is the operand or result of ptrtoint or inttoptr.

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.h

69

It might be worth a comment explaining why the memtype is also externref and funcref. Is this just for lack of a more meaningful type to return?

llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp

122–123

hasReferenceTypes should also be taking the CPU into account, not just the feature string. Normally this would be done via getSubtargetImpl, but I guess we probably can't call that this early in the construction of the WebAssemblyTargetMachine. Would anything break if we just unconditionally added the reference types address spaces to the data layout string?

[pmatos]

In D104797#2837417, @tlively wrote:

In D104797#2836475, @pmatos wrote:

@tlively Do you think it would be ok to re-add the code removed in ac81cb7e but only error if the pointer is to an opaque non-integral type?

Unfortunately that wouldn't be future-proof given the ongoing project to remove type information from pointers. I think the best we can do right now is call report_fatal_error from the backend whenever a reference type is the operand or result of ptrtoint or inttoptr.

Which 'ongoing project' is this and how can I obtain more information about it? Who's leading it?
Before the crashing, the only place we seem to touch the Wasm backend is in WebAssemblyDAGToDAGISel::runOnMachineFunction. Then this calls SelectionDAGISel::runOnMachineFunction, which visits each node in the Function and crashes when trying to call getPtrExtOrTrunc because it cannot create a TRUNCATE node for a zero sized type. So, if you prefer to have a report_fatal_error in the WebAssembly backend, we will need to visit the nodes in the function in WebAssemblyDAGToDAGISel::runOnMachineFunction before calling the generic runOnMachineFunction. I will look at how to proceed in this direction, but I am also interested in following this project on removing type information from pointers in LLVM, so any refs would be great. Thanks.

[tlively]

The opaque pointers project is documented here: https://llvm.org/docs/OpaquePointers.html. It's been making very slow progress for the past few years but has recently been picking up steam under the direction of @aeubanks. Search llvm-dev for "opaque pointers" and you will see a few recent threads about it.

[pmatos]

In D104797#2838653, @tlively wrote:

The opaque pointers project is documented here: https://llvm.org/docs/OpaquePointers.html. It's been making very slow progress for the past few years but has recently been picking up steam under the direction of @aeubanks. Search llvm-dev for "opaque pointers" and you will see a few recent threads about it.

Thanks for this.

[pmatos]

Checks for inttoptr and ptrtoint insts in wasm backend and bails out if necessary.

Also addresses @tlively's comments to the original patch.

[pmatos]

llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp

122–123

Regarding this change, I don't quite understand why referencetypes should take the CPU into account. Are there CPU variants for the wasm backend? I haven't touched the conditional because that would mean touching the several tests that don't enable reference types and use the old data layout string. However, I would think that if that's the path we want to follow here, we could do it and change all wasm tests to use the layout string with reference types.

[sbc100]

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp

1113

I was expecting to see a TABLE_SET back to a null after the call here to avoid the GC root being leaked in table slot 0 (as above). I must be missing something?

[tlively]

llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp

122–123

Yes, there are CPU variants defined here: https://github.com/llvm/llvm-project/blob/7ac0442fe59dbe0f9127e79e8786a7dd6345c537/llvm/lib/Target/WebAssembly/WebAssembly.td#L89-L100. Note that the CPU may enable reference types even if the feature string does not. If it doesn't break anything, then unconditionally updating the layout string sounds like the best option.

[pmatos]

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp

1113

@sbc100, that's what's happening in line 583.

[pmatos]

llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp

122–123

Interesting - had not come accross it. Bleeding edge does not seem to include reference-types. What's the reason for this?

[tlively]

llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp

122–123

We don't have a well-defined process for adding features to bleeding-edge, but I think typically they're added once they're mostly stable and usable in the tools.

[pmatos]

Unconditionally set Wasm layout string to include non-integral AS 10 and 20.

[pmatos]

llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp

122–123

@tlively I have now unconditionally updated the layout string. No failures. How does this look like now?

[pmatos]

Remove FIXME comment on data layout strings

[pmatos]

Make the linter happy

[pmatos]

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.h

69

As far as I understand that's the type in memory anyway, so it sounds like the right type to return in getPointerMemTy. I would think that if this question was for getPointerTy then the answer would be yes.

[tlively]

Thanks!

llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp

122–123

Looks good, thanks!

[lebedev.ri]

Reverted again

********************
FAIL: LLVM :: CodeGen/WebAssembly/funcref-call.ll (44466 of 44468)
******************** TEST 'LLVM :: CodeGen/WebAssembly/funcref-call.ll' FAILED ********************
Script:
--
: 'RUN: at line 1';   /builddirs/llvm-project/build-Clang12/bin/llc < /repositories/llvm-project/llvm/test/CodeGen/WebAssembly/funcref-call.ll --mtriple=wasm32-unknown-unknown -asm-verbose=false -mattr=+reference-types | /builddirs/llvm-project/build-Clang12/bin/FileCheck /repositories/llvm-project/llvm/test/CodeGen/WebAssembly/funcref-call.ll
--
Exit Code: 2

Command Output (stderr):
--
llc: /repositories/llvm-project/llvm/include/llvm/Support/LowLevelTypeImpl.h:44: static llvm::LLT llvm::LLT::scalar(unsigned int): Assertion `SizeInBits > 0 && "invalid scalar size"' failed.

Are you building llvm without assertions?

[pmatos]

In D104797#2854939, @lebedev.ri wrote:

Reverted again

********************
FAIL: LLVM :: CodeGen/WebAssembly/funcref-call.ll (44466 of 44468)
******************** TEST 'LLVM :: CodeGen/WebAssembly/funcref-call.ll' FAILED ********************
Script:
--
: 'RUN: at line 1';   /builddirs/llvm-project/build-Clang12/bin/llc < /repositories/llvm-project/llvm/test/CodeGen/WebAssembly/funcref-call.ll --mtriple=wasm32-unknown-unknown -asm-verbose=false -mattr=+reference-types | /builddirs/llvm-project/build-Clang12/bin/FileCheck /repositories/llvm-project/llvm/test/CodeGen/WebAssembly/funcref-call.ll
--
Exit Code: 2

Command Output (stderr):
--
llc: /repositories/llvm-project/llvm/include/llvm/Support/LowLevelTypeImpl.h:44: static llvm::LLT llvm::LLT::scalar(unsigned int): Assertion `SizeInBits > 0 && "invalid scalar size"' failed.

Are you building llvm without assertions?

I am sorry - thanks for reverting.
Last time I tested with check-all, nothing was failing. Then I updated to main to commit but because check-all takes a while and I didn't want to miss the boat to push, I pushed the commit.
However, the right procedure would have been probably to maybe run a quick llvm-lit on the WebAssembly tests. I didn't and between the main commit my patch was based on and current main HEAD a commit broke my patch again, specifically in sha 990278d026 (June 29).

I am now analysing this patch to reland, so I can fix mine once again and reland.

[pmatos]

After @arsenm 's commit https://github.com/llvm/llvm-project/commit/990278d026d680942c859be70836ad34a9a716f7, MachineOperands store LLT for size instead of uint64_t and the constructor called from SelectionDAG::getLoad, calls MachineMemOperand which does:

: MachineMemOperand(ptrinfo, f,
                    s == ~UINT64_C(0) ? LLT() : LLT::scalar(8 * s), a,
                    AAInfo, Ranges, SSID, Ordering, FailureOrdering) {}

Because s is zero for reference types, LLT::scalar(0) is called, which asserts because scalar LLTs cannot have zero size. Why do we have the check s == ~UINT64_C(0), instead of ss == UINT64_C(0) given opaque types and therefore reference types are zero sized?

[pmatos]

In D104797#2855322, @pmatos wrote:

After @arsenm 's commit https://github.com/llvm/llvm-project/commit/990278d026d680942c859be70836ad34a9a716f7, MachineOperands store LLT for size instead of uint64_t and the constructor called from SelectionDAG::getLoad, calls MachineMemOperand which does:

: MachineMemOperand(ptrinfo, f,
                    s == ~UINT64_C(0) ? LLT() : LLT::scalar(8 * s), a,
                    AAInfo, Ranges, SSID, Ordering, FailureOrdering) {}

Because s is zero for reference types, LLT::scalar(0) is called, which asserts because scalar LLTs cannot have zero size. Why do we have the check s == ~UINT64_C(0), instead of ss == UINT64_C(0) given opaque types and therefore reference types are zero sized?

OK, sort of replying to myself ~UINT64_T(0) is the size for invalid types. I think at this point we need LLT support for opaque types. Maybe we couldn't have opaque types stored in memory but we now can with reference types in WebAssembly.

[pmatos]

D105423 implements the required support in LLT of opaque types for this patch to work again.

[pmatos]

Remove some extraneous code

[pmatos]

In D104797#2864102, @pmatos wrote:

Remove some extraneous code

Argh, sorry. This was meant for another revision and I added code that's not meant for this revision.

[pmatos]

Revert code incorrectly submitted as part of diff

[pmatos]

@tlively once D105423 lands, is it enough to test and reland it under this revision or shall i open a new one?

[pmatos]

Remove hunks touching LLTs accidentally added before

[tlively]

In D104797#2879855, @pmatos wrote:

@tlively once D105423 lands, is it enough to test and reland it under this revision or shall i open a new one?

You can just reland it under this revision.

[pmatos]

Rebase on top of main and add non-integral address spaces to emscripten OS