PR24071 Fix an assert in SelectionDAGBuilder

[rovka]

Summary

When processing inline asm that contains errors, make sure we can recover gracefully by creating an UNDEF SDValue for the inline asm statement before returning from SelectionDAGBuilder::visitInlineAsm. This is necessary for consumers that don't exit on the first error that is emitted (e.g. clang) and that would assert later on.

Fixes PR24071.

[rovka]

The test is here: http://reviews.llvm.org/D20157

[t.p.northover]

It seems like it would be better to modify llc as you suggest. Adding an explicitly llc test to Clang is almost certainly the wrong thing to do.

Tim.

[rengolin]

In D20156#427155, @t.p.northover wrote:

It seems like it would be better to modify llc as you suggest. Adding an explicitly llc test to Clang is almost certainly the wrong thing to do.

Hi Tim,

That's a big change though. I agree llc should have diagnostics, too, but this bug shouldn't remain broken until it does, which can take a while and involve a lot of people.

I don't think having the Clang test is wrong, either. Just that it's better if the test is in the same repo and directly tested. I mean, even having an llc test, doesn't stop us from having a Clang one.

cheers,
--renato

[rovka]

In D20156#427155, @t.p.northover wrote:

It seems like it would be better to modify llc as you suggest. Adding an explicitly llc test to Clang is almost certainly the wrong thing to do.

Tim.

Thanks, Tim. I'll try to work on that.

[rnk]

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

6793–6799

Can you wrap up most of these calls into a helper like SelectionDAGBuilder::emitInlineAsmError(const Twine &) so that we don't forget to call setValue in the future?

[rovka]

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

6793–6799

Good idea. I'll post an updated version of this next week, after we get the new llc diagnostic handler (so I can add the tests too). Thanks.

[rovka]

Addressed previous comments.

Removed the exit-on-error flag on some of the CodeGen tests, that needed it in order to avoid this assert.

[rnk]

lgtm

[rengolin]

Committed in r269811.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

case InlineAsm::isOutput: {
         break;
       }
 
       // Otherwise, this is a register or register class output.
 
       // Copy the output from the appropriate register.  Find a register that
       // we can use.
       if (OpInfo.AssignedRegs.Regs.empty()) {
         LLVMContext &Ctx = *DAG.getContext();
         Ctx.emitError(CS.getInstruction(),
                       "couldn't allocate output register for constraint '" +
                           Twine(OpInfo.ConstraintCode) + "'");
+        auto VT = TLI.getValueType(DAG.getDataLayout(), CS.getType());
+        setValue(CS.getInstruction(), DAG.getUNDEF(VT));
         return;
       }
 
       // If this is an indirect operand, store through the pointer after the
       // asm.
       if (OpInfo.isIndirect) {
         IndirectStoresToEmit.push_back(std::make_pair(OpInfo.AssignedRegs,
                                                       OpInfo.CallOperandVal));
       } else {
case InlineAsm::isInput: {
             InlineAsm::isRegDefEarlyClobberKind(OpFlag)) {
           // Add (OpFlag&0xffff)>>3 registers to MatchedRegs.
           if (OpInfo.isIndirect) {
             // This happens on gcc/testsuite/gcc.dg/pr8788-1.c
             LLVMContext &Ctx = *DAG.getContext();
             Ctx.emitError(CS.getInstruction(), "inline asm not supported yet:"
                                                " don't know how to handle tied "
                                                "indirect register inputs");
+            auto VT = TLI.getValueType(DAG.getDataLayout(), CS.getType());
+            setValue(CS.getInstruction(), DAG.getUNDEF(VT));
             return;
           }
 
           RegsForValue MatchedRegs;
           MatchedRegs.ValueVTs.push_back(InOperandVal.getValueType());
           MVT RegVT = AsmNodeOperands[CurOp+1].getSimpleValueType();
           MatchedRegs.RegVTs.push_back(RegVT);
           MachineRegisterInfo &RegInfo = DAG.getMachineFunction().getRegInfo();
           for (unsigned i = 0, e = InlineAsm::getNumOperandRegisters(OpFlag);
                i != e; ++i) {
             if (const TargetRegisterClass *RC = TLI.getRegClassFor(RegVT))
               MatchedRegs.Regs.push_back(RegInfo.createVirtualRegister(RC));
             else {
               LLVMContext &Ctx = *DAG.getContext();
               Ctx.emitError(CS.getInstruction(),
                             "inline asm error: This value"
                             " type register class is not natively supported!");
+              auto VT = TLI.getValueType(DAG.getDataLayout(), CS.getType());
+              setValue(CS.getInstruction(), DAG.getUNDEF(VT));
               return;
             }
           }
           SDLoc dl = getCurSDLoc();
           // Use the produced MatchedRegs object to
           MatchedRegs.getCopyToRegs(InOperandVal, DAG, dl,
                                     Chain, &Flag, CS.getInstruction());
           MatchedRegs.AddInlineAsmOperands(InlineAsm::Kind_RegUse,
case InlineAsm::isInput: {
         std::vector<SDValue> Ops;
         TLI.LowerAsmOperandForConstraint(InOperandVal, OpInfo.ConstraintCode,
                                           Ops, DAG);
         if (Ops.empty()) {
           LLVMContext &Ctx = *DAG.getContext();
           Ctx.emitError(CS.getInstruction(),
                         "invalid operand for inline asm constraint '" +
                             Twine(OpInfo.ConstraintCode) + "'");
+          auto VT = TLI.getValueType(DAG.getDataLayout(), CS.getType());
+          setValue(CS.getInstruction(), DAG.getUNDEF(VT));
           return;
         }
 
         // Add information to the INLINEASM node to know about this input.
         unsigned ResOpType =
           InlineAsm::getFlagWord(InlineAsm::Kind_Imm, Ops.size());
         AsmNodeOperands.push_back(DAG.getTargetConstant(
             ResOpType, getCurSDLoc(), TLI.getPointerTy(DAG.getDataLayout())));
case InlineAsm::isInput: {
 
       // TODO: Support this.
       if (OpInfo.isIndirect) {
         LLVMContext &Ctx = *DAG.getContext();
         Ctx.emitError(CS.getInstruction(),
                       "Don't know how to handle indirect register inputs yet "
                       "for constraint '" +
                           Twine(OpInfo.ConstraintCode) + "'");
+        auto VT = TLI.getValueType(DAG.getDataLayout(), CS.getType());
+        setValue(CS.getInstruction(), DAG.getUNDEF(VT));
         return;
       }
 
       // Copy the input into the appropriate registers.
       if (OpInfo.AssignedRegs.Regs.empty()) {
         LLVMContext &Ctx = *DAG.getContext();
         Ctx.emitError(CS.getInstruction(),
                       "couldn't allocate input reg for constraint '" +
                           Twine(OpInfo.ConstraintCode) + "'");
+        auto VT = TLI.getValueType(DAG.getDataLayout(), CS.getType());
+        setValue(CS.getInstruction(), DAG.getUNDEF(VT));
         return;
       }
 
       SDLoc dl = getCurSDLoc();
 
       OpInfo.AssignedRegs.getCopyToRegs(InOperandVal, DAG, dl,
                                         Chain, &Flag, CS.getInstruction());