[WebAssembly] Produce error when encountering unlowerable Wasm global accesses

[asb]

Summary

WebAssembly globals are represented as IR globals with the wasm_var
address space (AS1). Prior to this patch, a wasm global load that isn't
lowerable will produce a failure to select, while a wasm global store
will produced incorrect code. This patch ensures we consistently produce
a clear error.

As noted in the test cases, it's conceivable that a frontend or an
optimisation pass could produce similar IR even in the presence of the
semantic restrictions on pointers to Wasm globals in the frontend, which
is a separate problem to address.

[sbc100]

llvm/test/CodeGen/WebAssembly/global-set-unlowerable.ll

23

Do these examples need to be this complex? Can you just elide of the branching? Why do we need to to different globals?

[asb]

Simplify test cases

[asb]

llvm/test/CodeGen/WebAssembly/global-set-unlowerable.ll

23

I've simplified from phi to select. The reason for two globals is the test case needs to demonstrate a case where the current ISel code won't match. Loading/storing a single global will work fine.

Arguably we should have coverage for loading/storing a global after getelementptr on it for completeness, but unlike these test cases that code _shouldn't_ occur unless the frontend fails to obey the semantic restrictions.

[pmatos]

LGTM

Indeed the tests for these are not comprehensive and these bugs haven't been found before as this sort of code hasn't been used in anger. I think the error message is fine for now but once the clang work is finished, we should refactor and improve this code.

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp

if (!Offset->isUndef())
                          false);
 
     SDValue Idx = DAG.getTargetConstant(*Local, Base, MVT::i32);
     SDVTList Tys = DAG.getVTList(MVT::Other); // The chain.
     SDValue Ops[] = {SN->getChain(), Idx, Value};
     return DAG.getNode(WebAssemblyISD::LOCAL_SET, DL, Tys, Ops);
   }
 
+  if (WebAssembly::isWasmVarAddressSpace(SN->getAddressSpace()))
+    report_fatal_error(
+        "Encountered an unlowerable store to the wasm_var address space",
+        false);
+
   return Op;
 }
 
 SDValue WebAssemblyTargetLowering::LowerLoad(SDValue Op,
                                              SelectionDAG &DAG) const {
   SDLoc DL(Op);
   LoadSDNode *LN = cast<LoadSDNode>(Op.getNode());
   const SDValue &Base = LN->getBasePtr();
if (Optional<unsigned> Local = IsWebAssemblyLocal(Base, DAG)) {
     EVT LocalVT = LN->getValueType(0);
     SDValue LocalGet = DAG.getNode(WebAssemblyISD::LOCAL_GET, DL, LocalVT,
                                    {LN->getChain(), Idx});
     SDValue Result = DAG.getMergeValues({LocalGet, LN->getChain()}, DL);
     assert(Result->getNumValues() == 2 && "Loads must carry a chain!");
     return Result;
   }
 
+  if (WebAssembly::isWasmVarAddressSpace(LN->getAddressSpace()))
+    report_fatal_error(
+        "Encountered an unlowerable load from the wasm_var address space",
+        false);
+
   return Op;
 }
 
 SDValue WebAssemblyTargetLowering::LowerCopyToReg(SDValue Op,
                                                   SelectionDAG &DAG) const {
   SDValue Src = Op.getOperand(2);
   if (isa<FrameIndexSDNode>(Src.getNode())) {
     // CopyToReg nodes don't support FrameIndex operands. Other targets select

llvm/test/CodeGen/WebAssembly/global-get-unlowerable.ll

+; RUN: not llc < %s --mtriple=wasm32 2>&1 | FileCheck %s
+
+; Demonstrates a code pattern that could be encountered even with frontend
+; restrictions on creating new pointers to globals. In the absence of a better
+; fix, the backend should produce a comprehensible message for why it can't
+; continue.
+
+; CHECK: LVM ERROR: Encountered an unlowerable load from the wasm_var address space
+
+@g1 = external addrspace(1) global i32
+@g2 = external addrspace(1) global i32
+
+define i32 @global_get_phi(i1 zeroext %bool) {
+  %sel = select i1 %bool, ptr addrspace(1) @g1, ptr addrspace(1) @g2
+  %gval = load i32, ptr addrspace(1) %sel
+  ret i32 %gval
+}

llvm/test/CodeGen/WebAssembly/global-set-unlowerable.ll

+; RUN: not llc < %s --mtriple=wasm32 2>&1 | FileCheck %s
+
+; Demonstrates a code pattern that could be encountered even with frontend
+; restrictions on creating new pointers to globals. In the absence of a better
+; fix, the backend should produce a comprehensible message for why it can't
+; continue.
+
+; CHECK: LVM ERROR: Encountered an unlowerable store to the wasm_var address space
+
+@g1 = external addrspace(1) global i32
+@g2 = external addrspace(1) global i32
+
+define void @global_set_phi(i1 zeroext %bool) {
+  %sel = select i1 %bool, ptr addrspace(1) @g1, ptr addrspace(1) @g2
+  store i32 100, ptr addrspace(1) %sel
+  ret void
+}