[X86] Fix crash with inline asm using wrong register name

[fwolff]

Summary

Fixes PR#48678. X86TargetLowering::getRegForInlineAsmConstraint() can adjust the register class to match the type, e.g. change VR128X to VR256X if the type needs 256 bits. However, the function currently returns the unadjusted register and the adjusted register class, e.g. xmm15 and VR256X, which then causes an assertion failure later because the register class does not contain that register. This patch fixes this behavior.

[pengfei]

I'm not sure if this is the right fix. The input somehow looks undefined behavior, which means we don't necessarily follow what's GCC doing. Besides, GCC doesn't handle it well in other case https://godbolt.org/z/6TTEvKvT6
Can we check for this case and simply report error/warning in the front end?

llvm/lib/Target/X86/X86ISelLowering.cpp

54134 ↗

(On Diff #387046)

Should be better to pass Res directly?

[fwolff]

I've changed it to issue an error instead now. Is this what you had in mind @pengfei?

[RKSimon]

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

8408

return None ?

[fwolff]

Use return None; instead of return {};.

[pengfei]

LGTM.

[fwolff]

Thanks again for the review. Can you also commit this for me @pengfei? (The libFuzzer test failure seems unrelated.)

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

 /// GetRegistersForValue - Assign registers (virtual or physical) for the
 /// specified operand.  We prefer to assign virtual registers, to allow the
 /// register allocator to handle the assignment process.  However, if the asm
 /// uses features that we can't model on machineinstrs, we have SDISel do the
 /// allocation.  This produces generally horrible, but correct, code.
 ///
 ///   OpInfo describes the operand
 ///   RefOpInfo describes the matching operand if any, the operand otherwise
-static void GetRegistersForValue(SelectionDAG &DAG, const SDLoc &DL,
-                                 SDISelAsmOperandInfo &OpInfo,
-                                 SDISelAsmOperandInfo &RefOpInfo) {
+static llvm::Optional<unsigned>
+getRegistersForValue(SelectionDAG &DAG, const SDLoc &DL,
   LLVMContext &Context = *DAG.getContext();
   const TargetLowering &TLI = DAG.getTargetLoweringInfo();
 
   MachineFunction &MF = DAG.getMachineFunction();
   SmallVector<unsigned, 4> Regs;
   const TargetRegisterInfo &TRI = *MF.getSubtarget().getRegisterInfo();
 
   // No work to do for memory operations.
   if (OpInfo.ConstraintType == TargetLowering::C_Memory)
-    return;
+    return None;
 
   // If this is a constraint for a single physreg, or a constraint for a
   // register class, find it.
   unsigned AssignedReg;
   const TargetRegisterClass *RC;
   std::tie(AssignedReg, RC) = TLI.getRegForInlineAsmConstraint(
       &TRI, RefOpInfo.ConstraintCode, RefOpInfo.ConstraintVT);
   // RC is unset only on failure. Return immediately.
   if (!RC)
-    return;
+    return None;
 
   // Get the actual register value type.  This is important, because the user
   // may have asked for (e.g.) the AX register in i32 type.  We need to
   // remember that AX is actually i16 to get the right extension.
   const MVT RegVT = *TRI.legalclasstypes_begin(*RC);
 
   if (OpInfo.ConstraintVT != MVT::Other && RegVT != MVT::Untyped) {
     // If this is an FP operand in an integer register (or visa versa), or more
if ((OpInfo.Type == InlineAsm::isOutput ||
         OpInfo.ConstraintVT = VT;
       }
     }
   }
 
   // No need to allocate a matching input constraint since the constraint it's
   // matching to has already been allocated.
   if (OpInfo.isMatchingInputConstraint())
-    return;
+    return None;
 
   EVT ValueVT = OpInfo.ConstraintVT;
   if (OpInfo.ConstraintVT == MVT::Other)
     ValueVT = RegVT;
 
   // Initialize NumRegs.
   unsigned NumRegs = 1;
   if (OpInfo.ConstraintVT != MVT::Other)
     NumRegs = TLI.getNumRegisters(Context, OpInfo.ConstraintVT, RegVT);
 
   // If this is a constraint for a specific physical register, like {r17},
   // assign it now.
 
   // If this associated to a specific register, initialize iterator to correct
   // place. If virtual, make sure we have enough registers
 
   // Initialize iterator if necessary
   TargetRegisterClass::iterator I = RC->begin();
   MachineRegisterInfo &RegInfo = MF.getRegInfo();
 
   // Do not check for single registers.
   if (AssignedReg) {
-      for (; *I != AssignedReg; ++I)
-        assert(I != RC->end() && "AssignedReg should be member of RC");
+    I = std::find(I, RC->end(), AssignedReg);
+    if (I == RC->end()) {
+      // RC does not contain the selected register, which indicates a
+      // mismatch between the register and the required type/bitwidth.
+      return {AssignedReg};
+    }
   }
 
   for (; NumRegs; --NumRegs, ++I) {
     assert(I != RC->end() && "Ran out of registers to allocate!");
     Register R = AssignedReg ? Register(*I) : RegInfo.createVirtualRegister(RC);
     Regs.push_back(R);
   }
 
   OpInfo.AssignedRegs = RegsForValue(Regs, RegVT, ValueVT);
+  return None;
 }
 
 static unsigned
 findMatchingInlineAsmOperand(unsigned OperandNo,
                              const std::vector<SDValue> &AsmNodeOperands) {
   // Scan until we find the definition we already emitted of this operand.
   unsigned CurOp = InlineAsm::Op_FirstOperand;
   for (; OperandNo; --OperandNo) {
void SelectionDAGBuilder::visitInlineAsm(const CallBase &Call,
   // Third pass: Loop over operands to prepare DAG-level operands.. As part of
   // this, assign virtual and physical registers for inputs and otput.
   for (SDISelAsmOperandInfo &OpInfo : ConstraintOperands) {
     // Assign Registers.
     SDISelAsmOperandInfo &RefOpInfo =
         OpInfo.isMatchingInputConstraint()
             ? ConstraintOperands[OpInfo.getMatchedOperand()]
             : OpInfo;
-    GetRegistersForValue(DAG, getCurSDLoc(), OpInfo, RefOpInfo);
+    const auto RegError =
+        getRegistersForValue(DAG, getCurSDLoc(), OpInfo, RefOpInfo);
+    if (RegError.hasValue()) {
+      const MachineFunction &MF = DAG.getMachineFunction();
+      const TargetRegisterInfo &TRI = *MF.getSubtarget().getRegisterInfo();
+      const char *RegName = TRI.getName(RegError.getValue());
+      emitInlineAsmError(Call, "register '" + Twine(RegName) +
+                                   "' allocated for constraint '" +
+                                   Twine(OpInfo.ConstraintCode) +
+                                   "' does not match required type");
+      return;
+    }
 
     auto DetectWriteToReservedRegister = [&]() {
       const MachineFunction &MF = DAG.getMachineFunction();
       const TargetRegisterInfo &TRI = *MF.getSubtarget().getRegisterInfo();
       for (unsigned Reg : OpInfo.AssignedRegs.Regs) {
         if (Register::isPhysicalRegister(Reg) &&
             TRI.isInlineAsmReadOnlyReg(MF, Reg)) {
           const char *RegName = TRI.getName(Reg);

llvm/test/CodeGen/X86/asm-reject-reg-type-mismatch-avx.ll

+; RUN: not llc -o /dev/null -mattr=avx %s 2>&1 | FileCheck %s
+target triple = "x86_64--"
+
+; CHECK: error: register 'XMM15' allocated for constraint '{xmm15}' does not match required type
+define void @test1() nounwind {
+entry:
+  tail call void asm sideeffect "call dummy", "{xmm15},~{dirflag},~{fpsr},~{flags}"(<8 x i32> <i32 1, i32 2, i32 3, i32 4, i32 5, i32 6, i32 7, i32 8>) #1
+  ret void
+}