Skip to content

[WebAssembly] Add path to PIC mode for wasm tables #67545

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Oct 3, 2023
Merged

[WebAssembly] Add path to PIC mode for wasm tables #67545

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
035945f
[WebAssembly] Add path to PIC mode for wasm tables
pmatos Sep 27, 2023
9f842f1
Add utility predicate for wasm table types
pmatos Sep 28, 2023
17a6de8
Run clang format.
pmatos Sep 28, 2023
61d83f7
Use predicate elsewhere to simplify code
pmatos Sep 28, 2023
0730f2b
Check for pointerty in reference types predicate.
pmatos Sep 29, 2023
93f749e
Remove unnecessary comment.
pmatos Oct 2, 2023
ad4836c
Run clang-format.
pmatos Oct 2, 2023
892d260
Update test
pmatos Oct 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions llvm/lib/Target/WebAssembly/Utils/WebAssemblyTypeUtilities.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,7 @@ void WebAssembly::wasmSymbolSetType(MCSymbolWasm *Sym, const Type *GlobalVT,
// that is a reference type.
wasm::ValType ValTy;
bool IsTable = false;
if (GlobalVT->isArrayTy() && WebAssembly::isWebAssemblyReferenceType(
GlobalVT->getArrayElementType())) {
if (WebAssembly::isWebAssemblyTableType(GlobalVT)) {
IsTable = true;
const Type *ElTy = GlobalVT->getArrayElementType();
if (WebAssembly::isWebAssemblyExternrefType(ElTy))
Expand Down
16 changes: 12 additions & 4 deletions llvm/lib/Target/WebAssembly/Utils/WebAssemblyTypeUtilities.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,21 +28,29 @@ namespace WebAssembly {

/// Return true if this is a WebAssembly Externref Type.
inline bool isWebAssemblyExternrefType(const Type *Ty) {
return Ty->getPointerAddressSpace() ==
WebAssembly::WasmAddressSpace::WASM_ADDRESS_SPACE_EXTERNREF;
return Ty->isPointerTy() &&
Ty->getPointerAddressSpace() ==
WebAssembly::WasmAddressSpace::WASM_ADDRESS_SPACE_EXTERNREF;
}

/// Return true if this is a WebAssembly Funcref Type.
inline bool isWebAssemblyFuncrefType(const Type *Ty) {
return Ty->getPointerAddressSpace() ==
WebAssembly::WasmAddressSpace::WASM_ADDRESS_SPACE_FUNCREF;
return Ty->isPointerTy() &&
Ty->getPointerAddressSpace() ==
WebAssembly::WasmAddressSpace::WASM_ADDRESS_SPACE_FUNCREF;
}

/// Return true if this is a WebAssembly Reference Type.
inline bool isWebAssemblyReferenceType(const Type *Ty) {
return isWebAssemblyExternrefType(Ty) || isWebAssemblyFuncrefType(Ty);
}

/// Return true if the table represents a WebAssembly table type.
inline bool isWebAssemblyTableType(const Type *Ty) {
return Ty->isArrayTy() &&
isWebAssemblyReferenceType(Ty->getArrayElementType());
}

// Convert StringRef to ValType / HealType / BlockType

MVT parseMVT(StringRef Type);
Expand Down
7 changes: 5 additions & 2 deletions llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1724,8 +1724,11 @@ SDValue WebAssemblyTargetLowering::LowerGlobalAddress(SDValue Op,
fail(DL, DAG, "Invalid address space for WebAssembly target");

unsigned OperandFlags = 0;
if (isPositionIndependent()) {
const GlobalValue *GV = GA->getGlobal();
const GlobalValue *GV = GA->getGlobal();
// Since WebAssembly tables cannot yet be shared accross modules, we don't
// need special treatment for tables in PIC mode.
if (isPositionIndependent() &&
!WebAssembly::isWebAssemblyTableType(GV->getValueType())) {
if (getTargetMachine().shouldAssumeDSOLocal(*GV->getParent(), GV)) {
MachineFunction &MF = DAG.getMachineFunction();
MVT PtrVT = getPointerTy(MF.getDataLayout());
Expand Down
12 changes: 10 additions & 2 deletions llvm/test/MC/WebAssembly/reloc-pic.s
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# RUN: llvm-mc -triple=wasm32-unknown-unknown -filetype=obj < %s | obj2yaml | FileCheck %s
# RUN: llvm-mc -triple=wasm32-unknown-unknown -mattr=+reference-types -filetype=obj < %s | obj2yaml | FileCheck --check-prefix=REF %s
# RUN: sed -e '/^REF-/d' %s | llvm-mc -triple=wasm32-unknown-unknown -filetype=obj | obj2yaml | FileCheck %s
# RUN: sed -e 's/^REF-//g' %s | llvm-mc -triple=wasm32-unknown-unknown -mattr=+reference-types -filetype=obj | obj2yaml | FileCheck --check-prefix=REF %s
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this something is OK to do in lit test? I've not seen this sed pattern before.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have at least test in WebAssembly (tls-local-exec.ll) using sed. There are many in RISCV which uses sed to replace types depending on current running arch, etc. We need this here to ensure that without reference types we don't add a table and if we do have reference types, the result is correct. Alternatively we need another test file. I am happy to do that if you're not keen on sed but sed is very much used together with lit tests in general.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will wait for a further thumbs up from you if you're happy with introducing this sed pattern into the tests.


# Verify that @GOT relocation entryes result in R_WASM_GLOBAL_INDEX_LEB against
# against the corrsponding function or data symbol and that the corresponding
Expand Down Expand Up @@ -50,6 +50,9 @@ hidden_func:
#.hidden hidden_data
.size default_data, 4

REF-mytable:
REF-.tabletype mytable, externref

# CHECK: --- !WASM
# CHECK-NEXT: FileHeader:
# CHECK-NEXT: Version: 0x1
Expand Down Expand Up @@ -209,6 +212,11 @@ hidden_func:
# CHECK-NEXT: Function: 5
# REF: - Index: 10
# REF-NEXT: Kind: TABLE
# REF-NEXT: Name: mytable
# REF-NEXT: Flags: [ BINDING_LOCAL ]
# REF-NEXT: Table: 1
# REF-NEXT: - Index: 11
# REF-NEXT: Kind: TABLE
# REF-NEXT: Name: __indirect_function_table
# REF-NEXT: Flags: [ UNDEFINED, NO_STRIP ]
# REF-NEXT: Table: 0
Expand Down