[wasm] Fix FastISel generating NoReg

[ddcc]

Summary

Previously, FastISel for WebAssembly wasn't checking the return value of getRegForValue in certain cases, which would generate instructions referencing NoReg. This patch fixes this behavior.

[dschuff]

test/CodeGen/WebAssembly/noreg.ll

1

This command line shouldn't result in fast-isel even running, should it?

11

are these constexprs (and inttoptrs/casts) necessary to reproduce?
Is the underlying problem is that the getRegForValue calls for the icmp are bailing, or something like that?

[ddcc]

test/CodeGen/WebAssembly/noreg.ll

1

Portions of this pass are getting called through e.g. the generic FastISel.cpp:1417 and SelectionDAGISel.cpp:1369 for selecting return instructions.

11

Yes, I believe they are necessary. The issue is that when optimization has been turned off in the front-end, instruction can have sufficiently complex operands that fail inside getRegForValue during FastISel, which will return zero. But previously we weren't checking these return values, putting NoReg into the generated instructions and causing problems later.

[dschuff]

test/CodeGen/WebAssembly/noreg.ll

1

Hm, I didn't realize that any of this ran when not using fast-isel. In any case we should also add a second RUN line that includes -fast-isel to explicitly get as much of fast-isel as possible.

3

This description doesn't really make sense; the fact that NoReg operands are generated is the bug. Really what is happening is that there are parts that cannot be selected by fast-isel but it doesn't properly bail out.

[dschuff]

test/CodeGen/WebAssembly/noreg.ll

3

Oh, probably the corollary to "fix the comment to say what the tested condition and expected result is" would also be to change the name of the test file to something that reflects the tested condition.

[ddcc]

Rename test, update comment, add -fast-isel

[dschuff]

OK, looks good. one more request, please expand the commit description a little bit.

lib/Target/WebAssembly/WebAssemblyFastISel.cpp

unsigned WebAssemblyFastISel::getRegForI1Value(const Value *V, bool &Not) {
   }
 
   Not = false;
   return maskI1Value(getRegForValue(V), V);
 }
 
 unsigned WebAssemblyFastISel::zeroExtendToI32(unsigned Reg, const Value *V,
                                               MVT::SimpleValueType From) {
+  if (Reg == 0)
+    return 0;
+
   switch (From) {
   case MVT::i1:
     // If the value is naturally an i1, we don't need to mask it.
     // TODO: Recursively examine selects, phis, and, or, xor, constants.
     if (From == MVT::i1 && V != nullptr) {
       if (isa<CmpInst>(V) ||
           (isa<Argument>(V) && cast<Argument>(V)->hasZExtAttr()))
         return copyValue(Reg);
BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc,
     .addReg(Reg)
     .addReg(Imm);
 
   return Result;
 }
 
 unsigned WebAssemblyFastISel::signExtendToI32(unsigned Reg, const Value *V,
                                               MVT::SimpleValueType From) {
+  if (Reg == 0)
+    return 0;
+
   switch (From) {
   case MVT::i1:
   case MVT::i8:
   case MVT::i16:
     break;
   case MVT::i32:
     return copyValue(Reg);
   default:
case MVT::f64:
     RC = &WebAssembly::F64RegClass;
     break;
   default: return false;
   }
 
   materializeLoadStoreOperands(Addr);
 
   unsigned ValueReg = getRegForValue(Store->getValueOperand());
+  if (ValueReg == 0)
+    return false;
   if (VTIsi1)
     ValueReg = maskI1Value(ValueReg, Store->getValueOperand());
 
   unsigned ResultReg = createResultReg(RC);
   auto MIB = BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc, TII.get(Opc),
                      ResultReg);
 
   addLoadStoreOperands(Addr, MIB, createMachineMemOperandFor(Store));
if (Br->isUnconditional()) {
     return true;
   }
 
   MachineBasicBlock *TBB = FuncInfo.MBBMap[Br->getSuccessor(0)];
   MachineBasicBlock *FBB = FuncInfo.MBBMap[Br->getSuccessor(1)];
 
   bool Not;
   unsigned CondReg = getRegForI1Value(Br->getCondition(), Not);
+  if (CondReg == 0)
+    return false;
 
   unsigned Opc = WebAssembly::BR_IF;
   if (Not)
     Opc = WebAssembly::BR_UNLESS;
 
   BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc, TII.get(Opc))
       .addMBB(TBB)
       .addReg(CondReg);
-  
   finishCondBranch(Br->getParent(), TBB, FBB);
   return true;
 }
 
 bool WebAssemblyFastISel::selectRet(const Instruction *I) {
   if (!FuncInfo.CanLowerReturn)
     return false;
 
bool WebAssemblyFastISel::selectRet(const Instruction *I) {
   unsigned Reg;
   if (FuncInfo.Fn->getAttributes().hasAttribute(0, Attribute::SExt))
     Reg = getRegForSignedValue(RV);
   else if (FuncInfo.Fn->getAttributes().hasAttribute(0, Attribute::ZExt))
     Reg = getRegForUnsignedValue(RV);
   else
     Reg = getRegForValue(RV);
 
+  if (Reg == 0)
+    return false;
+
   BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc, TII.get(Opc)).addReg(Reg);
   return true;
 }
 
 bool WebAssemblyFastISel::selectUnreachable(const Instruction *I) {
   BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc,
           TII.get(WebAssembly::UNREACHABLE));
   return true;

test/CodeGen/WebAssembly/noreg.ll

+; RUN: llc < %s -asm-verbose=false -disable-wasm-fallthrough-return-opt -verify-machineinstrs | FileCheck %s
+
+; Test that NoReg is handled correctly by FastISel
+
+target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
+target triple = "wasm32-unknown-unknown"
+
+; CHECK: i32.const $push0=, 0
+define hidden i32 @a() #0 {
+entry:
+  ret i32 zext (i1 icmp eq (void (...)* inttoptr (i32 10 to void (...)*), void (...)* null) to i32)
+}
+
+; CHECK: i32.const $push0=, 1
+; CHECK: br_if 0, $pop0
+define hidden i32 @b() #0 {
+entry:
+  br i1 icmp eq (void (...)* inttoptr (i32 10 to void (...)*), void (...)* null), label %a, label %b
+a:
+  unreachable
+b:
+  ret i32 0
+}
+
+; CHECK: i32.const $push1=, 0
+; CHECK: i32.const $push2=, 0
+; CHECK: i32.store $drop=, 0($pop1), $pop2
+define hidden i32 @c() #0 {
+entry:
+  store i32 zext (i1 icmp eq (void (...)* inttoptr (i32 10 to void (...)*), void (...)* null) to i32), i32* inttoptr (i32 0 to i32 *)
+  ret i32 0
+}
+
+attributes #0 = { noinline optnone }