[WebAssembly] Implement lowering calls through funcref to call_ref when available #162227
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-backend-webassembly Author: Demetrius Kanios (QuantumSegfault) ChangesAllows calls through Builds upon the framework provided by #147486 Example Source IR define void @<!-- -->call_ref_void(%funcref %callee) {
call addrspace(20) void %callee()
ret void
}Result Before this PR and/or without GC: i32.const 0
local.get 0
table.set __funcref_call_table
i32.const 0
call_indirect __funcref_call_table, () -> ()
i32.const 0
ref.null_func
table.set __funcref_call_tableAfter this PR, when compiled with local.get 0
ref.cast () -> ()
call_ref () -> ()Full diff: https://github.com/llvm/llvm-project/pull/162227.diff 8 Files Affected:
diff --git a/llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h b/llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h
index fe9a4bada2430..db4d9edb152ce 100644
--- a/llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h
+++ b/llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h
@@ -435,6 +435,18 @@ inline bool isCallIndirect(unsigned Opc) {
}
}
+inline bool isCallRef(unsigned Opc) {
+ switch (Opc) {
+ case WebAssembly::CALL_REF:
+ case WebAssembly::CALL_REF_S:
+ case WebAssembly::RET_CALL_REF:
+ case WebAssembly::RET_CALL_REF_S:
+ return true;
+ default:
+ return false;
+ }
+}
+
inline bool isBrTable(unsigned Opc) {
switch (Opc) {
case WebAssembly::BR_TABLE_I32:
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp
index 2541b0433ab59..03c90c7160a68 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp
@@ -120,60 +120,6 @@ static SDValue getTagSymNode(int Tag, SelectionDAG *DAG) {
return DAG->getTargetExternalSymbol(SymName, PtrVT);
}
-static APInt encodeFunctionSignature(SelectionDAG *DAG, SDLoc &DL,
- SmallVector<MVT, 4> &Returns,
- SmallVector<MVT, 4> &Params) {
- auto toWasmValType = [](MVT VT) {
- if (VT == MVT::i32) {
- return wasm::ValType::I32;
- }
- if (VT == MVT::i64) {
- return wasm::ValType::I64;
- }
- if (VT == MVT::f32) {
- return wasm::ValType::F32;
- }
- if (VT == MVT::f64) {
- return wasm::ValType::F64;
- }
- if (VT == MVT::externref) {
- return wasm::ValType::EXTERNREF;
- }
- if (VT == MVT::funcref) {
- return wasm::ValType::FUNCREF;
- }
- if (VT == MVT::exnref) {
- return wasm::ValType::EXNREF;
- }
- LLVM_DEBUG(errs() << "Unhandled type for llvm.wasm.ref.test.func: " << VT
- << "\n");
- llvm_unreachable("Unhandled type for llvm.wasm.ref.test.func");
- };
- auto NParams = Params.size();
- auto NReturns = Returns.size();
- auto BitWidth = (NParams + NReturns + 2) * 64;
- auto Sig = APInt(BitWidth, 0);
-
- // Annoying special case: if getSignificantBits() <= 64 then InstrEmitter will
- // emit an Imm instead of a CImm. It simplifies WebAssemblyMCInstLower if we
- // always emit a CImm. So xor NParams with 0x7ffffff to ensure
- // getSignificantBits() > 64
- Sig |= NReturns ^ 0x7ffffff;
- for (auto &Return : Returns) {
- auto V = toWasmValType(Return);
- Sig <<= 64;
- Sig |= (int64_t)V;
- }
- Sig <<= 64;
- Sig |= NParams;
- for (auto &Param : Params) {
- auto V = toWasmValType(Param);
- Sig <<= 64;
- Sig |= (int64_t)V;
- }
- return Sig;
-}
-
void WebAssemblyDAGToDAGISel::Select(SDNode *Node) {
// If we have a custom node, we already have selected!
if (Node->isMachineOpcode()) {
@@ -288,7 +234,8 @@ void WebAssemblyDAGToDAGISel::Select(SDNode *Node) {
Returns.push_back(VT);
}
}
- auto Sig = encodeFunctionSignature(CurDAG, DL, Returns, Params);
+ auto Sig =
+ WebAssembly::encodeFunctionSignature(CurDAG, DL, Returns, Params);
auto SigOp = CurDAG->getTargetConstant(
Sig, DL, EVT::getIntegerVT(*CurDAG->getContext(), Sig.getBitWidth()));
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
index 163bf9ba5b089..bd0733c73f7ed 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
@@ -723,6 +723,7 @@ LowerCallResults(MachineInstr &CallResults, DebugLoc DL, MachineBasicBlock *BB,
bool IsIndirect =
CallParams.getOperand(0).isReg() || CallParams.getOperand(0).isFI();
bool IsRetCall = CallResults.getOpcode() == WebAssembly::RET_CALL_RESULTS;
+ bool IsCallRef = false;
bool IsFuncrefCall = false;
if (IsIndirect && CallParams.getOperand(0).isReg()) {
@@ -732,10 +733,19 @@ LowerCallResults(MachineInstr &CallResults, DebugLoc DL, MachineBasicBlock *BB,
const TargetRegisterClass *TRC = MRI.getRegClass(Reg);
IsFuncrefCall = (TRC == &WebAssembly::FUNCREFRegClass);
assert(!IsFuncrefCall || Subtarget->hasReferenceTypes());
+
+ if (IsFuncrefCall && Subtarget->hasGC()) {
+ IsIndirect = false;
+ IsCallRef = true;
+ }
}
unsigned CallOp;
- if (IsIndirect && IsRetCall) {
+ if (IsCallRef && IsRetCall) {
+ CallOp = WebAssembly::RET_CALL_REF;
+ } else if (IsCallRef) {
+ CallOp = WebAssembly::CALL_REF;
+ } else if (IsIndirect && IsRetCall) {
CallOp = WebAssembly::RET_CALL_INDIRECT;
} else if (IsIndirect) {
CallOp = WebAssembly::CALL_INDIRECT;
@@ -771,6 +781,14 @@ LowerCallResults(MachineInstr &CallResults, DebugLoc DL, MachineBasicBlock *BB,
CallParams.addOperand(FnPtr);
}
+ // Move the function pointer to the end of the arguments for funcref calls
+ if (IsCallRef) {
+ auto FnRef = CallParams.getOperand(0);
+ CallParams.removeOperand(0);
+
+ CallParams.addOperand(FnRef);
+ }
+
for (auto Def : CallResults.defs())
MIB.add(Def);
@@ -795,6 +813,12 @@ LowerCallResults(MachineInstr &CallResults, DebugLoc DL, MachineBasicBlock *BB,
}
}
+ if (IsCallRef) {
+ // Placeholder for the type index.
+ // This gets replaced with the correct value in WebAssemblyMCInstLower.cpp
+ MIB.addImm(0);
+ }
+
for (auto Use : CallParams.uses())
MIB.add(Use);
@@ -1173,6 +1197,60 @@ static bool callingConvSupported(CallingConv::ID CallConv) {
CallConv == CallingConv::Swift;
}
+APInt WebAssembly::encodeFunctionSignature(SelectionDAG *DAG, SDLoc &DL,
+ SmallVector<MVT, 4> &Returns,
+ SmallVector<MVT, 4> &Params) {
+ auto toWasmValType = [](MVT VT) {
+ if (VT == MVT::i32) {
+ return wasm::ValType::I32;
+ }
+ if (VT == MVT::i64) {
+ return wasm::ValType::I64;
+ }
+ if (VT == MVT::f32) {
+ return wasm::ValType::F32;
+ }
+ if (VT == MVT::f64) {
+ return wasm::ValType::F64;
+ }
+ if (VT == MVT::externref) {
+ return wasm::ValType::EXTERNREF;
+ }
+ if (VT == MVT::funcref) {
+ return wasm::ValType::FUNCREF;
+ }
+ if (VT == MVT::exnref) {
+ return wasm::ValType::EXNREF;
+ }
+ LLVM_DEBUG(errs() << "Unhandled type for llvm.wasm.ref.test.func: " << VT
+ << "\n");
+ llvm_unreachable("Unhandled type for llvm.wasm.ref.test.func");
+ };
+ auto NParams = Params.size();
+ auto NReturns = Returns.size();
+ auto BitWidth = (NParams + NReturns + 2) * 64;
+ auto Sig = APInt(BitWidth, 0);
+
+ // Annoying special case: if getSignificantBits() <= 64 then InstrEmitter will
+ // emit an Imm instead of a CImm. It simplifies WebAssemblyMCInstLower if we
+ // always emit a CImm. So xor NParams with 0x7ffffff to ensure
+ // getSignificantBits() > 64
+ Sig |= NReturns ^ 0x7ffffff;
+ for (auto &Return : Returns) {
+ auto V = toWasmValType(Return);
+ Sig <<= 64;
+ Sig |= (int64_t)V;
+ }
+ Sig <<= 64;
+ Sig |= NParams;
+ for (auto &Param : Params) {
+ auto V = toWasmValType(Param);
+ Sig <<= 64;
+ Sig |= (int64_t)V;
+ }
+ return Sig;
+}
+
SDValue
WebAssemblyTargetLowering::LowerCall(CallLoweringInfo &CLI,
SmallVectorImpl<SDValue> &InVals) const {
@@ -1412,33 +1490,58 @@ WebAssemblyTargetLowering::LowerCall(CallLoweringInfo &CLI,
InTys.push_back(In.VT);
}
- // Lastly, if this is a call to a funcref we need to add an instruction
- // table.set to the chain and transform the call.
+ // Lastly, if this is a call to a funcref we need to insert an instruction
+ // to either cast the funcref to a typed funcref for call_ref, or place it
+ // into a table for call_indirect
if (CLI.CB && WebAssembly::isWebAssemblyFuncrefType(
CLI.CB->getCalledOperand()->getType())) {
- // In the absence of function references proposal where a funcref call is
- // lowered to call_ref, using reference types we generate a table.set to set
- // the funcref to a special table used solely for this purpose, followed by
- // a call_indirect. Here we just generate the table set, and return the
- // SDValue of the table.set so that LowerCall can finalize the lowering by
- // generating the call_indirect.
- SDValue Chain = Ops[0];
+ if (Subtarget->hasGC()) {
+ // Since LLVM doesn't directly support typed function references, we take
+ // the untyped funcref and ref.cast it into a typed funcref.
+ SmallVector<MVT, 4> Params;
+ SmallVector<MVT, 4> Returns;
+
+ for (const auto &Out : Outs) {
+ Params.push_back(Out.VT);
+ }
+ for (const auto &In : Ins) {
+ Returns.push_back(In.VT);
+ }
- MCSymbolWasm *Table = WebAssembly::getOrCreateFuncrefCallTableSymbol(
- MF.getContext(), Subtarget);
- SDValue Sym = DAG.getMCSymbol(Table, PtrVT);
- SDValue TableSlot = DAG.getConstant(0, DL, MVT::i32);
- SDValue TableSetOps[] = {Chain, Sym, TableSlot, Callee};
- SDValue TableSet = DAG.getMemIntrinsicNode(
- WebAssemblyISD::TABLE_SET, DL, DAG.getVTList(MVT::Other), TableSetOps,
- MVT::funcref,
- // Machine Mem Operand args
- MachinePointerInfo(
- WebAssembly::WasmAddressSpace::WASM_ADDRESS_SPACE_FUNCREF),
- CLI.CB->getCalledOperand()->getPointerAlignment(DAG.getDataLayout()),
- MachineMemOperand::MOStore);
-
- Ops[0] = TableSet; // The new chain is the TableSet itself
+ auto Sig =
+ WebAssembly::encodeFunctionSignature(&DAG, DL, Returns, Params);
+
+ auto SigOp = DAG.getTargetConstant(
+ Sig, DL, EVT::getIntegerVT(*DAG.getContext(), Sig.getBitWidth()));
+ MachineSDNode *RefCastNode = DAG.getMachineNode(
+ WebAssembly::REF_CAST_FUNCREF, DL, MVT::funcref, {SigOp, Callee});
+
+ Ops[1] = SDValue(RefCastNode, 0);
+ } else {
+ // In the absence of function references proposal where a funcref call is
+ // lowered to call_ref, using reference types we generate a table.set to
+ // set the funcref to a special table used solely for this purpose,
+ // followed by a call_indirect. Here we just generate the table set, and
+ // return the SDValue of the table.set so that LowerCall can finalize the
+ // lowering by generating the call_indirect.
+ SDValue Chain = Ops[0];
+
+ MCSymbolWasm *Table = WebAssembly::getOrCreateFuncrefCallTableSymbol(
+ MF.getContext(), Subtarget);
+ SDValue Sym = DAG.getMCSymbol(Table, PtrVT);
+ SDValue TableSlot = DAG.getConstant(0, DL, MVT::i32);
+ SDValue TableSetOps[] = {Chain, Sym, TableSlot, Callee};
+ SDValue TableSet = DAG.getMemIntrinsicNode(
+ WebAssemblyISD::TABLE_SET, DL, DAG.getVTList(MVT::Other), TableSetOps,
+ MVT::funcref,
+ // Machine Mem Operand args
+ MachinePointerInfo(
+ WebAssembly::WasmAddressSpace::WASM_ADDRESS_SPACE_FUNCREF),
+ CLI.CB->getCalledOperand()->getPointerAlignment(DAG.getDataLayout()),
+ MachineMemOperand::MOStore);
+
+ Ops[0] = TableSet; // The new chain is the TableSet itself
+ }
}
if (CLI.IsTailCall) {
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.h b/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.h
index b33a8530310be..7d2194132f293 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.h
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.h
@@ -141,6 +141,11 @@ class WebAssemblyTargetLowering final : public TargetLowering {
namespace WebAssembly {
FastISel *createFastISel(FunctionLoweringInfo &funcInfo,
const TargetLibraryInfo *libInfo);
+
+APInt encodeFunctionSignature(SelectionDAG *DAG, SDLoc &DL,
+ SmallVector<MVT, 4> &Returns,
+ SmallVector<MVT, 4> &Params);
+
} // end namespace WebAssembly
} // end namespace llvm
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyInstrCall.td b/llvm/lib/Target/WebAssembly/WebAssemblyInstrCall.td
index ca9a5ef9dda1c..81b62f6a682ec 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyInstrCall.td
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyInstrCall.td
@@ -66,6 +66,16 @@ defm CALL_INDIRECT :
[],
"call_indirect", "call_indirect\t$type, $table", 0x11>;
+let variadicOpsAreDefs = 1 in
+defm CALL_REF :
+ I<(outs),
+ (ins TypeIndex:$type, variable_ops),
+ (outs),
+ (ins TypeIndex:$type),
+ [],
+ "call_ref", "call_ref\t$type", 0x14>,
+ Requires<[HasGC]>;
+
let isReturn = 1, isTerminator = 1, hasCtrlDep = 1, isBarrier = 1 in
defm RET_CALL :
I<(outs), (ins function32_op:$callee, variable_ops),
@@ -81,4 +91,14 @@ defm RET_CALL_INDIRECT :
0x13>,
Requires<[HasTailCall]>;
+let isReturn = 1, isTerminator = 1, hasCtrlDep = 1, isBarrier = 1 in
+defm RET_CALL_REF :
+ I<(outs),
+ (ins TypeIndex:$type, variable_ops),
+ (outs),
+ (ins TypeIndex:$type),
+ [],
+ "return_call_ref", "return_call_ref\t$type", 0x15>,
+ Requires<[HasTailCall, HasGC]>;
+
} // Uses = [SP32,SP64], isCall = 1
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyInstrRef.td b/llvm/lib/Target/WebAssembly/WebAssemblyInstrRef.td
index fc82e5b4a61da..6fa6ed897d647 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyInstrRef.td
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyInstrRef.td
@@ -41,6 +41,11 @@ defm REF_TEST_FUNCREF : I<(outs I32:$res), (ins TypeIndex:$type, FUNCREF:$ref),
"ref.test\t$type, $ref", "ref.test $type", 0xfb14>,
Requires<[HasGC]>;
+defm REF_CAST_FUNCREF : I<(outs FUNCREF:$res), (ins TypeIndex:$type, FUNCREF:$ref),
+ (outs), (ins TypeIndex:$type), [],
+ "ref.cast\t$type, $ref", "ref.cast $type", 0xfb16>,
+ Requires<[HasGC]>;
+
defm "" : REF_I<FUNCREF, funcref, "func">;
defm "" : REF_I<EXTERNREF, externref, "extern">;
defm "" : REF_I<EXNREF, exnref, "exn">;
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp
index e48283aadb437..1ed15967c01fe 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp
@@ -230,7 +230,7 @@ void WebAssemblyMCInstLower::lower(const MachineInstr *MI,
break;
}
case llvm::MachineOperand::MO_CImmediate: {
- // Lower type index placeholder for ref.test
+ // Lower type index placeholder for ref.test and ref.cast
// Currently this is the only way that CImmediates show up so panic if we
// get confused.
unsigned DescIndex = I - NumVariadicDefs;
@@ -266,14 +266,16 @@ void WebAssemblyMCInstLower::lower(const MachineInstr *MI,
Params.push_back(WebAssembly::regClassToValType(
MRI.getRegClass(MO.getReg())->getID()));
- // call_indirect instructions have a callee operand at the end which
- // doesn't count as a param.
- if (WebAssembly::isCallIndirect(MI->getOpcode()))
+ // call_indirect and call_ref instructions have a callee operand at
+ // the end which doesn't count as a param.
+ if (WebAssembly::isCallIndirect(MI->getOpcode()) ||
+ WebAssembly::isCallRef(MI->getOpcode()))
Params.pop_back();
- // return_call_indirect instructions have the return type of the
- // caller
- if (MI->getOpcode() == WebAssembly::RET_CALL_INDIRECT)
+ // return_call_indirect and return_call_ref instructions have the
+ // return type of the caller
+ if (MI->getOpcode() == WebAssembly::RET_CALL_INDIRECT ||
+ MI->getOpcode() == WebAssembly::RET_CALL_REF)
getFunctionReturns(MI, Returns);
MCOp = lowerTypeIndexOperand(std::move(Returns), std::move(Params));
diff --git a/llvm/test/CodeGen/WebAssembly/call-ref.ll b/llvm/test/CodeGen/WebAssembly/call-ref.ll
new file mode 100644
index 0000000000000..25fc7440ac64c
--- /dev/null
+++ b/llvm/test/CodeGen/WebAssembly/call-ref.ll
@@ -0,0 +1,51 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 6
+; RUN: llc < %s -mattr=+reference-types,-gc | FileCheck --check-prefixes=CHECK,NOGC %s
+; RUN: llc < %s -mattr=+reference-types,+gc | FileCheck --check-prefixes=CHECK,GC %s
+
+; Test that calls through funcref lower to call_ref when GC is available
+
+target triple = "wasm32-unknown-unknown"
+
+%funcref = type ptr addrspace(20);
+
+define void @call_ref_void(%funcref %callee) {
+; CHECK-LABEL: call_ref_void:
+; CHECK: .functype call_ref_void (funcref) -> ()
+; CHECK-NEXT: # %bb.0:
+; NOGC-NEXT: i32.const 0
+; CHECK-NEXT: local.get 0
+; NOGC-NEXT: table.set __funcref_call_table
+; NOGC-NEXT: i32.const 0
+; NOGC-NEXT: call_indirect __funcref_call_table, () -> ()
+; NOGC-NEXT: i32.const 0
+; NOGC-NEXT: ref.null_func
+; NOGC-NEXT: table.set __funcref_call_table
+; GC-NEXT: ref.cast () -> ()
+; GC-NEXT: call_ref () -> ()
+; CHECK-NEXT: # fallthrough-return
+ call addrspace(20) void %callee()
+ ret void
+}
+
+define void @call_ref_with_args_and_ret(%funcref %callee) {
+; CHECK-LABEL: call_ref_with_args_and_ret:
+; CHECK: .functype call_ref_with_args_and_ret (funcref) -> ()
+; CHECK-NEXT: # %bb.0:
+; NOGC-NEXT: i32.const 0
+; NOGC-NEXT: local.get 0
+; NOGC-NEXT: table.set __funcref_call_table
+; CHECK-NEXT: i32.const 1
+; CHECK-NEXT: f64.const 0x1p1
+; NOGC-NEXT: i32.const 0
+; NOGC-NEXT: call_indirect __funcref_call_table, (i32, f64) -> (i32)
+; GC-NEXT: local.get 0
+; GC-NEXT: ref.cast (i32, f64) -> (i32)
+; GC-NEXT: call_ref (i32, f64) -> (i32)
+; CHECK-NEXT: drop
+; NOGC-NEXT: i32.const 0
+; NOGC-NEXT: ref.null_func
+; NOGC-NEXT: table.set __funcref_call_table
+; CHECK-NEXT: # fallthrough-return
+ %result = call addrspace(20) i32 %callee(i32 1, double 2.0)
+ ret void
+}
|
dschuff
left a comment
dschuff
left a comment
There was a problem hiding this comment.
Since you are adding a new instruction I think we also need MC tests that cover assembling them, (like the tests in test/llvm/MC/WebAssembly) so we can cover things like encodings of instructions (and e.g. check out the tests that cover use of __indirect_function table).
| ; NOGC-NEXT: i32.const 0 | ||
| ; NOGC-NEXT: ref.null_func | ||
| ; NOGC-NEXT: table.set __funcref_call_table | ||
| ; GC-NEXT: ref.cast () -> () |
There was a problem hiding this comment.
@tlively I assume there is never an advantage to the NOGC version of this compared to the GC version? even if call_indirect is a bit faster than cast+call_ref, it still seems nicer than this dance with the tables.
|
What's the use case for this? Same application you mentioned in #155880 ? |
No. I have no concrete need or use case for this right now. It's just something I noticed as I was building up GlobalISel. Since WasmGC is now part of the core spec (wasm 3.0), and the foundation was already put in place #147486, I saw no reason not to make the improvement. |
Oh, I didn't know. I was following the pattern of, again, #147486 I'll look into writing an appropriate test. Should I also test |
|
That would be great. I think I just missed it before. Maybe test/MC/WebAssembly/reference-types.s would be a good place. |
|
That makes more sense: #139642 is the one that actually added the instruction and the corresponding tests. But there, AsmParser support (which I realized was missing after seeing these tests) and tests for the three new instructions. |
You can test this locally with the following command:git-clang-format --diff origin/main HEAD --extensions cpp,h -- llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h llvm/lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.h llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp
View the diff from clang-format here.diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.h b/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.h
index 61ccaec49..6851a7637 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.h
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.h
@@ -143,8 +143,7 @@ FastISel *createFastISel(FunctionLoweringInfo &funcInfo,
const TargetLibraryInfo *libInfo);
APInt encodeFunctionSignature(SelectionDAG *DAG, SDLoc &DL,
- ArrayRef<MVT> Returns,
- ArrayRef<MVT> Params);
+ ArrayRef<MVT> Returns, ArrayRef<MVT> Params);
} // end namespace WebAssembly
|
So the change looks good to me now; the only concerns I have would be that I assume this means you haven't tested it in any larger system? I guess the number of users for this is pretty small, so we may not know anytime soon if there are bugs. |
|
Fair points. And you are correct, I haven't tested it. So I tried to build a minimum test to ensure that the two lowerings are comparable at runtime, and I run into this when trying to compile it with the old/existing lowering (-gc, or with older LLVM): I tested the following the Clang/LLVM 20 Compiling this file with #define WASM_EXPORT __attribute__((visibility("default")))
#ifdef __cplusplus
extern "C" {
#endif
typedef void (*my_func_ptr)(int);
WASM_EXPORT void test_func(__funcref my_func_ptr callback) {
callback(32);
}
#ifdef __cplusplus
}
#endifYields this IR: ; ModuleID = 'call_ref_test.bc'
source_filename = "call_ref_test.c"
target datalayout = "e-m:e-p:32:32-p10:8:8-p20:8:8-i64:64-i128:128-n32:64-S128-ni:1:10:20"
target triple = "wasm32"
; Function Attrs: nounwind
define void @test_func(ptr addrspace(20) noundef readonly captures(none) %0) local_unnamed_addr #0 {
tail call addrspace(20) void %0(i32 noundef 32) #1
ret void
}
attributes #0 = { nounwind "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="generic" "target-features"="+bulk-memory,+bulk-memory-opt,+call-indirect-overlong,+multivalue,+mutable-globals,+nontrapping-fptoint,+reference-types,+sign-ext" }
attributes #1 = { nounwind }
!llvm.module.flags = !{!0}
!llvm.ident = !{!1}
!0 = !{i32 1, !"wchar_size", i32 4}
!1 = !{!"clang version 20.1.8"}So far so good. Compile to S with .file "call_ref_test.c"
.tabletype __funcref_call_table, funcref, 1
.functype test_func (funcref) -> ()
.section .text.test_func,"",@
.globl test_func # -- Begin function test_func
.type test_func,@function
test_func: # @test_func
.functype test_func (funcref) -> ()
# %bb.0:
i32.const 0
local.get 0
table.set __funcref_call_table
i32.const 32
i32.const 0
call_indirect __funcref_call_table, (i32) -> ()
i32.const 0
ref.null_func
table.set __funcref_call_table
# fallthrough-return
end_function
# -- End functionStill perfectly fine. However as soon as I augment the During llvm-project/llvm/lib/MC/WasmObjectWriter.cpp Lines 1397 to 1409 in 3f3ffed So did I do something wrong here, or did I stumble across a bug? If I did, I'm not seeing a bug report, so it seems you are right in that "the number of users for this is pretty small" |
|
Yeah, I'm pretty sure you stumbled across a bug. What happens if you take that .s file and try to assemble it with llvm-mc? It looks right (cf. this test). Maybe there's some field on the MCSymbolWasm that's not getting initialized or something like that? |
|
...it works? It compiles just fine If I run that through (module $-
(type (;0;) (func (param i32)))
(type (;1;) (func (param funcref)))
(import "env" "__funcref_call_table" (table (;0;) 0 funcref))
(memory (;0;) 2)
(global $__stack_pointer (;0;) (mut i32) (i32.const 66560))
(export "memory" (memory 0))
(export "__stack_pointer" (global $__stack_pointer))
(export "test_func" (func $test_func))
(func $test_func (;0;) (type 1) (param funcref)
(table.set 0
(i32.const 0)
(local.get 0))
(call_indirect (type 0)
(i32.const 32)
(i32.const 0))
(table.set 0
(i32.const 0)
(ref.null func))
)
(@producers
(processed-by "clang" "20.1.8")
)
(@custom "target_features" (after code) "\08+\0bbulk-memory+\0fbulk-memory-opt+\16call-indirect-overlong+\0amultivalue+\0fmutable-globals+\13nontrapping-fptoint+\0freference-types+\08sign-ext")
)The one thing that sticks out is that --- !WASM
FileHeader:
Version: 0x1
Sections:
- Type: TYPE
Signatures:
- Index: 0
ParamTypes:
- FUNCREF
ReturnTypes: []
- Index: 1
ParamTypes:
- I32
ReturnTypes: []
- Type: IMPORT
Imports:
- Module: env
Field: __linear_memory
Kind: MEMORY
Memory:
Minimum: 0x0
- Module: env
Field: __funcref_call_table
Kind: TABLE
Table:
Index: 0
ElemType: FUNCREF
Limits:
Minimum: 0x0
- Type: FUNCTION
FunctionTypes: [ 0 ]
- Type: CODE
Relocations:
- Type: R_WASM_TABLE_NUMBER_LEB
Index: 1
Offset: 0x8
- Type: R_WASM_TYPE_INDEX_LEB
Index: 1
Offset: 0x12
- Type: R_WASM_TABLE_NUMBER_LEB
Index: 1
Offset: 0x17
- Type: R_WASM_TABLE_NUMBER_LEB
Index: 1
Offset: 0x21
Functions:
- Index: 0
Locals: []
Body: 410020002680808080004120410011818080800080808080004100D0702680808080000B
- Type: CUSTOM
Name: linking
Version: 2
SymbolTable:
- Index: 0
Kind: FUNCTION
Name: test_func
Flags: [ ]
Function: 0
- Index: 1
Kind: TABLE
Name: __funcref_call_table
Flags: [ UNDEFINED ]
Table: 0
- Type: CUSTOM
Name: producers
Tools:
- Name: clang
Version: 20.1.8
- Type: CUSTOM
Name: target_features
Features:
- Prefix: USED
Name: bulk-memory
- Prefix: USED
Name: bulk-memory-opt
- Prefix: USED
Name: call-indirect-overlong
- Prefix: USED
Name: multivalue
- Prefix: USED
Name: mutable-globals
- Prefix: USED
Name: nontrapping-fptoint
- Prefix: USED
Name: reference-types
- Prefix: USED
Name: sign-extThe symbol is created weak, just like the |
|
Yeah, I think there's a couple of different possibilities for how this should work in a toolchain. You could have every object file that implicitly uses the table just import it. Then at link time you'd need to either have the linker implicitly create the table in the linked binary, or you can let the table be imported (and then you'd require it to be imported from JS or the environment). lld has a flag that lets you choose between these for regarding the specific error you're getting, what I meant above was that I suspect that when the MCSymbolWasm gets created from parsing an asm file, you end up with a regular undefined symbol that assembles fine, but when that same module gets created by the compiler rather than by the asm parser, the object isn't fully initialized and it ends up weak by default. |
|
Alright, so after poking around at it some more, I think it ultimately comes down to the fact that the The symbol is marked with "setWeak" as if the table was designed to be fully defined in EACH object file, with the linker just choosing one (all of them are the same, with a single default-initialized So we either need a way to define/initialize the table per-object (which I'm struggling to find; just a simple way to mark it "defined"...which seems to require a
Its not weak by default. During creation, it is explicitly marked weak: llvm-project/llvm/lib/Target/WebAssembly/WebAssemblyUtilities.cpp Lines 123 to 146 in 11bf901 |
3 participants
Allows calls through
funcref(ptr addrspace(20)) to be lowered to a sequence ofref.cast+call_refwhen WasmGC is available. This is opposed to the current work around of storing the funcref into a special table, and usingcall_indirect.Builds upon the framework provided by #147486
Example
Source IR
Result
Before this PR and/or without GC:
After this PR, when compiled with
-mattr=+gc: