[X86][MC] Assert unexpected form in emitREXPrefix, NFCI

[skan]

Summary

1. Add a variable HasRegOp to record if the instruction has a register operand
2. Enumerate all the formats with a register operand in the switch
3. Add a default (unreachable) label in the switch (suggested by @reames)

[pengfei]

llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp

1200

Not sure others, but should this be unreachable too?

1257

Move this ahead?

[skan]

Combine duplicated code

[skan]

llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp

1200

It is reachable b/c the format can have an implicit register operand. If we removed this, the following command would crash

echo "movabsq 0x123456789abcdef, %rax" | llvm-mc --show-encoding

where movabsq is MOV64ao64 in TD

[pengfei]

llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp

1197

You intend to !NumOps || !HasRegOp?

[skan]

Address review comments

[skan]

llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp

1197

!NumOps || !HasRegOp is same as !HasRegOp b/c !NumOps implies !HasRegOp

[pengfei]

llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp

1199

How about move the check here:

if (HasRegOp)
  llvm_unreachable("Unexpected form in emitREXPrefix!");

Then you don't need the lambda expression.

[skan]

simplify code

[skan]

llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp

1199

I haven't got your idea. Lambda is used to check HighByteReg. How can we achieve the same effect with your code?

[pengfei]

llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp

1199

Because you don't need to do the check and return twice with the change. Just do it in the end of the function.

[skan]

llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp

1199

Got it, will do

[skan]

Address review comments: avoid lambda

[pengfei]

LGTM.

llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp

1201

We don't need the else here.

1202

This can also be [[fallthrough]];

[skan]

Address review comments: use fallthrough

[skan]

Fix the compiler-rt test fail by adding format RawFrm

[craig.topper]

llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp

1200

assert(!HasRegOp && "Unexpected form in emitREXPrefix!")

[skan]

Address review comments: simplify code by using assert

[skan]

Add macro NDEBUG b/c the variable HasRegOp is only used in assert

llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp

 ///
 /// \returns the used prefix (REX or None).
 PrefixKind X86MCCodeEmitter::emitREXPrefix(int MemOperand, const MCInst &MI,
                                            const MCSubtargetInfo &STI,
                                            raw_ostream &OS) const {
   if (!STI.hasFeature(X86::Is64Bit))
     return None;
   X86OpcodePrefixHelper Prefix(*Ctx.getRegisterInfo());
-  bool UsesHighByteReg = false;
   const MCInstrDesc &Desc = MCII.get(MI.getOpcode());
   uint64_t TSFlags = Desc.TSFlags;
   Prefix.setW(TSFlags & X86II::REX_W);
   unsigned NumOps = MI.getNumOperands();
-  if (!NumOps) {
-    PrefixKind Kind = Prefix.determineOptimalKind();
-    Prefix.emit(OS);
-    return Kind;
-  }
-  unsigned CurOp = X86II::getOperandBias(Desc);
+  bool UsesHighByteReg = false;
+#ifndef NDEBUG
+  bool HasRegOp = false;
+#endif
+  unsigned CurOp = NumOps ? X86II::getOperandBias(Desc) : 0;
   for (unsigned i = CurOp; i != NumOps; ++i) {
     const MCOperand &MO = MI.getOperand(i);
     if (MO.isReg()) {
+#ifndef NDEBUG
+      HasRegOp = true;
+#endif
       unsigned Reg = MO.getReg();
       if (Reg == X86::AH || Reg == X86::BH || Reg == X86::CH || Reg == X86::DH)
         UsesHighByteReg = true;
       // If it accesses SPL, BPL, SIL, or DIL, then it requires a REX prefix.
       if (X86II::isX86_64NonExtLowByteReg(Reg))
         Prefix.setLowerBound(REX);
     } else if (MO.isExpr() && STI.getTargetTriple().isX32()) {
       // GOTTPOFF and TLSDESC relocations require a REX prefix to allow
       // linker optimizations: even if the instructions we see may not require
       // any prefix, they may be replaced by instructions that do. This is
       // handled as a special case here so that it also works for hand-written
       // assembly without the user needing to write REX, as with GNU as.
       const auto *Ref = dyn_cast<MCSymbolRefExpr>(MO.getExpr());
       if (Ref && (Ref->getKind() == MCSymbolRefExpr::VK_GOTTPOFF ||
                   Ref->getKind() == MCSymbolRefExpr::VK_TLSDESC)) {
         Prefix.setLowerBound(REX);
       }
     }
   }
   switch (TSFlags & X86II::FormMask) {
+  default:
+    assert(!HasRegOp && "Unexpected form in emitREXPrefix!");
+  case X86II::RawFrm:
+  case X86II::RawFrmMemOffs:
+  case X86II::RawFrmSrc:
+  case X86II::RawFrmDst:
+  case X86II::RawFrmDstSrc:
+    break;
   case X86II::AddRegFrm:
     Prefix.setB(MI, CurOp++);
     break;
   case X86II::MRMSrcReg:
   case X86II::MRMSrcRegCC:
     Prefix.setR(MI, CurOp++);
     Prefix.setB(MI, CurOp++);
     break;
#endif
   case X86II::MRM3r:
   case X86II::MRM4r:
   case X86II::MRM5r:
   case X86II::MRM6r:
   case X86II::MRM7r:
     Prefix.setB(MI, CurOp++);
     break;
   }
   PrefixKind Kind = Prefix.determineOptimalKind();
   if (Kind && UsesHighByteReg)
     report_fatal_error(
         "Cannot encode high byte register in REX-prefixed instruction");
   Prefix.emit(OS);
   return Kind;
 }
 
 /// Emit segment override opcode prefix as needed.