[X86] Error on using h-registers with REX prefix in the assembler instead of leaving it to a fatal error in the encoder.

[craig.topper]

Summary

Using a fatal error is bad for user experience.

[pengfei]

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp

3632

It seems X86MCCodeEmitter::emitREXPrefix has more detailed check for each form. Especially, the commens says "Emit REX prefix ... 2) non-default operand size". Is it not covered by this case?

[craig.topper]

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp

3632

I'm not sure what the difference is between "1) 64-bit instructions" and "2) non-default operand size".

I think the form code is just mapping registers to different bits in the REX prefix. I don't think the assembler error needs to care about the exact bits, just that some bit will be affected by the operands. But I might be wrong.

[craig.topper]

Skip the REX prefix check if encoding is VEX/EVEX/XOP.

[pengfei]

LGTM. Thanks.

[MaskRay]

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp

3641

LG.

minor nit: https://llvm.org/docs/CodingStandards.html#error-and-warning-messages the majority of error messages don't end with a period.

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp

bool X86AsmParser::MatchRegisterByName(unsigned &RegNo, StringRef RegName,
   if (isParsingMSInlineAsm() && isParsingIntelSyntax() &&
       (RegNo == X86::EFLAGS || RegNo == X86::MXCSR))
     RegNo = 0;
 
   if (!is64BitMode()) {
     // FIXME: This should be done using Requires<Not64BitMode> and
     // Requires<In64BitMode> so "eiz" usage in 64-bit instructions can be also
     // checked.
-    // FIXME: Check AH, CH, DH, BH cannot be used in an instruction requiring a
-    // REX prefix.
     if (RegNo == X86::RIZ || RegNo == X86::RIP ||
         X86MCRegisterClasses[X86::GR64RegClassID].contains(RegNo) ||
         X86II::isX86_64NonExtLowByteReg(RegNo) ||
         X86II::isX86_64ExtendedReg(RegNo)) {
       return Error(StartLoc,
                    "register %" + RegName + " is only available in 64-bit mode",
                    SMRange(StartLoc, EndLoc));
     }
if (Src2Enc % 4 != 0) {
                      RegName.take_front(3) + Twine(GroupStart) + "' to '" +
                      RegName.take_front(3) + Twine(GroupEnd) +
                      "' source group");
     }
     break;
   }
   }
 
+  const MCInstrDesc &MCID = MII.get(Inst.getOpcode());
+  // Check that we aren't mixing AH/BH/CH/DH with REX prefix. We only need to
+  // check this with the legacy encoding, VEX/EVEX/XOP don't use REX.
+  if ((MCID.TSFlags & X86II::EncodingMask) == 0) {
+    MCPhysReg HReg = X86::NoRegister;
+    bool UsesRex = MCID.TSFlags & X86II::REX_W;
+    unsigned NumOps = Inst.getNumOperands();
+    for (unsigned i = 0; i != NumOps; ++i) {
+      const MCOperand &MO = Inst.getOperand(i);
+      if (!MO.isReg())
+        continue;
+      unsigned Reg = MO.getReg();
+      if (Reg == X86::AH || Reg == X86::BH || Reg == X86::CH || Reg == X86::DH)
+        HReg = Reg;
+      if (X86II::isX86_64NonExtLowByteReg(Reg) ||
+          X86II::isX86_64ExtendedReg(Reg))
+        UsesRex = true;
+    }
+
+    if (UsesRex && HReg != X86::NoRegister) {
+      StringRef RegName = X86IntelInstPrinter::getRegisterName(HReg);
+      return Error(Ops[0]->getStartLoc(),
+                   "can't encode '" + RegName + "' in an instruction requiring "
+                   "REX prefix.");
+    }
+  }
+
   return false;
 }
 
 static const char *getSubtargetFeatureName(uint64_t Val);
 
 void X86AsmParser::emitWarningForSpecialLVIInstruction(SMLoc Loc) {
   Warning(Loc, "Instruction may be vulnerable to LVI and "
                "requires manual mitigation");
bool X86AsmParser::MatchAndEmitATTInstruction(SMLoc IDLoc, unsigned &Opcode,
   Op.setTokenValue(Base);
 
   // If exactly one matched, then we treat that as a successful match (and the
   // instruction will already have been filled in correctly, since the failing
   // matches won't have modified it).
   unsigned NumSuccessfulMatches =
       std::count(std::begin(Match), std::end(Match), Match_Success);
   if (NumSuccessfulMatches == 1) {
+    if (!MatchingInlineAsm && validateInstruction(Inst, Operands))
+      return true;
     // Some instructions need post-processing to, for example, tweak which
     // encoding is selected. Loop on it while changes happen so the
     // individual transformations can chain off each other.
     if (!MatchingInlineAsm)
       while (processInstruction(Inst, Operands))
         ;
 
     Inst.setLoc(IDLoc);

llvm/test/MC/X86/encoder-fail.s

-// RUN: not --crash llvm-mc -triple x86_64-unknown-unknown --show-encoding %s 2>&1 | FileCheck %s
-// CHECK: LLVM ERROR: Cannot encode high byte register in REX-prefixed instruction
- movzx %dh, %rsi
+// RUN: not llvm-mc -triple x86_64-unknown-unknown --show-encoding %s 2>&1 | FileCheck %s
+
+// CHECK: error: can't encode 'dh' in an instruction requiring REX prefix.
+
+// CHECK: error: can't encode 'ah' in an instruction requiring REX prefix.
+movzx %ah, %r8d
+
+// CHECK: error: can't encode 'bh' in an instruction requiring REX prefix.
+add %bh, %sil
+
+// CHECK: error: can't encode 'ch' in an instruction requiring REX prefix.
+mov %ch, (%r8)
+
+// CHECK: error: can't encode 'dh' in an instruction requiring REX prefix.
+mov %dh, (%rax,%r8)