[WebAssembly] Implement ReplaceNodeResults to fix a SIMD crash

[aheejin]

We talked offline and it's maybe better to have a bit more picture on how ReplaceNodeResults is working before committing this.

llvm/test/CodeGen/WebAssembly/simd-illegal-signext.ll

8 ↗

(On Diff #196315)

Nit: Can we make it wrap to 80 cols bit more? It currently looks kinda random

15 ↗

(On Diff #196315)

Nit: we can remove #0

[tlively]

DAGTypeLegalizer calls ReplaceNodeResults whenever CustomLowerNode is called and its parameter LegalizeResult is true or when CustomWidenLowerNode is called. So basically it is the same as LowerOperation except that it is meant to focus on legalizing the result type rather than simply transforming the operation. There is even a TODO on LowerOperationWrapper, /// TODO: Consider merging with ReplaceNodeResults..

llvm/test/CodeGen/WebAssembly/simd-illegal-signext.ll

8 ↗

(On Diff #196315)

I would have to break up the very long WebAssemblyTargetLowering::ReplaceNodeResults to make the wrapping prettier. I think it's more valuable to keep the name so the reader knows exactly what was fixed.

[tlively]

• Remove hanging metadata tag in test

[aheejin]

Thanks for investigating this! This is getting kind of hard to understand without knowing the context that why signext_inreg is custom lowered for SIMD but actually not custom lowered....

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp

898 ↗

(On Diff #197906)

Can we guarantee this function is only executed in case of SIGN_EXTEND_INREG? How about checking its opcode here and if not do llvm_unreachable or something, with some comments why we need this for SIGN_EXTEND_INREG?

[tlively]

• Fail if trying to run ReplaceNode on an unexpected op

llvm/trunk/lib/Target/WebAssembly/WebAssemblyISelLowering.h

SDValue LowerReturn(SDValue Chain, CallingConv::ID CallConv, bool isVarArg,
                       const SmallVectorImpl<SDValue> &OutVals, const SDLoc &dl,
                       SelectionDAG &DAG) const override;
   SDValue LowerFormalArguments(SDValue Chain, CallingConv::ID CallConv,
                                bool IsVarArg,
                                const SmallVectorImpl<ISD::InputArg> &Ins,
                                const SDLoc &DL, SelectionDAG &DAG,
                                SmallVectorImpl<SDValue> &InVals) const override;
 
+  void ReplaceNodeResults(SDNode *N, SmallVectorImpl<SDValue> &Results,
+                          SelectionDAG &DAG) const override;
+
   // Custom lowering hooks.
   SDValue LowerOperation(SDValue Op, SelectionDAG &DAG) const override;
   SDValue LowerFrameIndex(SDValue Op, SelectionDAG &DAG) const;
   SDValue LowerRETURNADDR(SDValue Op, SelectionDAG &DAG) const;
   SDValue LowerFRAMEADDR(SDValue Op, SelectionDAG &DAG) const;
   SDValue LowerGlobalAddress(SDValue Op, SelectionDAG &DAG) const;
   SDValue LowerExternalSymbol(SDValue Op, SelectionDAG &DAG) const;
   SDValue LowerBR_JT(SDValue Op, SelectionDAG &DAG) const;

llvm/trunk/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp

SDValue WebAssemblyTargetLowering::LowerFormalArguments(
   // the param logic here with ComputeSignatureVTs
   assert(MFI->getParams().size() == Params.size() &&
          std::equal(MFI->getParams().begin(), MFI->getParams().end(),
                     Params.begin()));
 
   return Chain;
 }
 
+void WebAssemblyTargetLowering::ReplaceNodeResults(
+    SDNode *N, SmallVectorImpl<SDValue> &Results, SelectionDAG &DAG) const {
+  switch (N->getOpcode()) {
+  case ISD::SIGN_EXTEND_INREG:
+    // Do not add any results, signifying that N should not be custom lowered
+    // after all. This happens because simd128 turns on custom lowering for
+    // SIGN_EXTEND_INREG, but for non-vector sign extends the result might be an
+    // illegal type.
+    break;
+  default:
+    llvm_unreachable(
+        "ReplaceNodeResults not implemented for this op for WebAssembly!");
+  }
+}
+
 //===----------------------------------------------------------------------===//
 //  Custom lowering hooks.
 //===----------------------------------------------------------------------===//
 
 SDValue WebAssemblyTargetLowering::LowerOperation(SDValue Op,
                                                   SelectionDAG &DAG) const {
   SDLoc DL(Op);
   switch (Op.getOpcode()) {

llvm/trunk/test/CodeGen/WebAssembly/simd-illegal-signext.ll

+; RUN: llc < %s -mattr=+simd128 | FileCheck %s
+
+; Regression test for a crash caused by
+; WebAssemblyTargetLowering::ReplaceNodeResults not being
+; implemented. Since SIMD is enabled, sign_ext_inreg is custom lowered
+; but the result is i16, an illegal value. This requires
+; ReplaceNodeResults to resolve, but the default implementation is to
+; abort.
+
+target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
+target triple = "wasm32-unknown-emscripten"
+
+; CHECK: i32.load8_s
+; CHECK-NEXT: i32.store16
+define void @foo() {
+entry:
+  %0 = load i32*, i32** undef, align 4
+  %1 = load i32, i32* %0, align 4
+  %2 = load i32, i32* undef, align 4
+  %conv67 = trunc i32 %2 to i8
+  %conv68 = sext i8 %conv67 to i16
+  store i16 %conv68, i16* null, align 2
+  ret void
+}