Skip to content
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.

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

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

Merged
merged 8 commits into from
Oct 3, 2023

Conversation

pmatos
Copy link
Contributor

@pmatos pmatos commented Sep 27, 2023

Currently tables cannot be shared between compilation units, therefore
no special treatment is needed for tables.

Fixes #65191

@pmatos pmatos requested a review from sbc100 September 27, 2023 11:52
@pmatos pmatos self-assigned this Sep 27, 2023
@llvmbot llvmbot added backend:WebAssembly mc Machine (object) code labels Sep 27, 2023
@pmatos pmatos removed the mc Machine (object) code label Sep 27, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Sep 27, 2023

@llvm/pr-subscribers-backend-webassembly

@llvm/pr-subscribers-mc

Changes

Currently tables cannot be shared between compilation units, therefore
no special treatment is needed for tables.

Fixes #65191


Full diff: https://github.com/llvm/llvm-project/pull/67545.diff

2 Files Affected:

  • (modified) llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp (+8-2)
  • (modified) llvm/test/MC/WebAssembly/reloc-pic.s (+10-2)
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
index fd154a90edef1d9..b67615d82d04a54 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
@@ -1724,8 +1724,14 @@ 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();
+  // is PIC but not a WebAssembly table.
+  // Since WebAssembly tables cannot yet be shared accross modules, we don't need special
+  // treatment for tables in PIC mode.
+  if (isPositionIndependent() && 
+      !(GV->getValueType()->isArrayTy() && 
+        WebAssembly::isWebAssemblyReferenceType(GV->getValueType()->getArrayElementType()))) {
+    
     if (getTargetMachine().shouldAssumeDSOLocal(*GV->getParent(), GV)) {
       MachineFunction &MF = DAG.getMachineFunction();
       MVT PtrVT = getPointerTy(MF.getDataLayout());
diff --git a/llvm/test/MC/WebAssembly/reloc-pic.s b/llvm/test/MC/WebAssembly/reloc-pic.s
index dd5d315cc2ce03f..1758820da93479a 100644
--- a/llvm/test/MC/WebAssembly/reloc-pic.s
+++ b/llvm/test/MC/WebAssembly/reloc-pic.s
@@ -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
 
 # Verify that @GOT relocation entryes result in R_WASM_GLOBAL_INDEX_LEB against
 # against the corrsponding function or data symbol and that the corresponding
@@ -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
@@ -209,6 +212,11 @@ hidden_func:
 # CHECK-NEXT:         Function:        5
 # REF:              - Index:           10
 # REF-NEXT:           Kind:            TABLE
+# REF-NEXT:           Name:            mytable
+# REF-NEXT:           Flags:           [ UNDEFINED ]
+# 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

@github-actions
Copy link

github-actions bot commented Sep 27, 2023

✅ With the latest revision this PR passed the C/C++ code formatter.

const GlobalValue *GV = GA->getGlobal();
// is PIC but not a WebAssembly table.
// Since WebAssembly tables cannot yet be shared accross modules, we don't need special
// treatment for tables in PIC mode.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you saying that shouldAssumeDSOLocal() is always true for table symbols?

Why can't tables be imported/exported like any other symbol?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As it stands tables need to be always declared with static. Symbols cannot be exported. I will get around to implementing it at some point to fix #64532.

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp Outdated Show resolved Hide resolved
@llvmbot llvmbot added the mc Machine (object) code label Sep 28, 2023
@pmatos pmatos requested a review from sbc100 September 28, 2023 13:35
Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

lgtm % a couple more comments

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp Outdated Show resolved Hide resolved
# 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.

@pmatos pmatos requested a review from sbc100 October 2, 2023 10:07
@pmatos pmatos merged commit a29e8ef into llvm:main Oct 3, 2023
3 checks passed
@pmatos pmatos deleted the WasmTablePIC branch October 3, 2023 06:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:WebAssembly mc Machine (object) code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

clang "cannot select" when using wasm table builtins and -fPIC
3 participants