Page MenuHomePhabricator

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

Authored by pmatos on Jun 23 2021, 10:38 AM.

Details

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.

Diff Detail

Event Timeline

pmatos created this revision.Jun 23 2021, 10:38 AM
pmatos requested review of this revision.Jun 23 2021, 10:38 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 23 2021, 10:38 AM

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 added a subscriber: reames.Jun 23 2021, 10:45 AM

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 added a comment.EditedJun 23 2021, 4:18 PM

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

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

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.

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 updated this revision to Diff 354945.Jun 28 2021, 10:22 AM

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

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

pmatos added inline comments.Jun 28 2021, 12:29 PM
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 added inline comments.Jun 28 2021, 12:53 PM
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 added inline comments.Jun 28 2021, 1:24 PM
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 added inline comments.Jun 30 2021, 5:41 AM
llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
1113

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

pmatos added inline comments.Jun 30 2021, 5:43 AM
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 added inline comments.Jun 30 2021, 9:45 AM
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 updated this revision to Diff 355792.Jul 1 2021, 12:40 AM

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

pmatos added inline comments.Jul 1 2021, 12:41 AM
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 updated this revision to Diff 355793.Jul 1 2021, 12:42 AM

Remove FIXME comment on data layout strings

pmatos updated this revision to Diff 355830.Jul 1 2021, 4:21 AM

Make the linter happy

pmatos added inline comments.Jul 1 2021, 7:22 AM
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 accepted this revision.Jul 1 2021, 12:48 PM

Thanks!

llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp
122–123

Looks good, thanks!

This revision is now accepted and ready to land.Jul 1 2021, 12:48 PM
This revision was landed with ongoing or failed builds.Jul 2 2021, 12:46 AM
This revision was automatically updated to reflect the committed changes.
lebedev.ri reopened this revision.Jul 2 2021, 1:50 AM
lebedev.ri added a subscriber: 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?

This revision is now accepted and ready to land.Jul 2 2021, 1:50 AM
pmatos added a comment.Jul 2 2021, 6:59 AM

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 added a subscriber: arsenm.Jul 2 2021, 7:02 AM
pmatos added a comment.EditedJul 2 2021, 7:33 AM

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 added a comment.Jul 2 2021, 7:48 AM

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.

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

pmatos updated this revision to Diff 357183.Jul 8 2021, 3:46 AM

Remove some extraneous code

pmatos added a comment.Jul 8 2021, 3:54 AM

Remove some extraneous code

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

pmatos updated this revision to Diff 357189.Jul 8 2021, 4:17 AM

Revert code incorrectly submitted as part of diff

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

pmatos updated this revision to Diff 359258.Jul 16 2021, 2:27 AM

Remove hunks touching LLTs accidentally added before

@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 updated this revision to Diff 360055.Jul 20 2021, 2:06 AM

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

aeubanks removed a subscriber: aeubanks.Jul 20 2021, 8:26 AM
This revision was landed with ongoing or failed builds.Jul 22 2021, 1:07 PM
This revision was automatically updated to reflect the committed changes.