[X86][MC][NFC] Reduce the parameters of functions in X86MCCodeEmitter(Part II)

[skan]

Summary

We determine the REX prefix used by instruction in determineREXPrefix,
and this value is used in `emitMemModRMByte' and used as the return
value of emitOpcodePrefix.

Before this patch, REX was passed as reference to emitPrefixImpl, it
is strange and not necessary, e.g, we have to write

bool Rex = false;
emitPrefixImpl(CurOp, CurByte, Rex, MI, STI, OS);

in emitPrefix even if Rex will not be used.

So we let HasREX be the return value of emitPrefixImpl. The HasREX is passed
from emitREXPrefix to emitOpcodePrefix and then to
emitPrefixImpl. This makes sense since REX is a kind of opcode prefix
and of course is a prefix.

[craig.topper]

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

1401

Why not return it as a bool from emitREXPrefix?

[craig.topper]

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

1283

Don't we need to check that REX is non-zero before emitting the byte? Am I missing something?

[skan]

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

1401

To myself, it seems more nature to return a REX ... . I am not sure if furture instruction's encoding will depends on the value of REX. Do you mean it is better to return REX as bool?

[skan]

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

1283

you are right. I missed this

[skan]

Address part of review comments
Check the REX is not zero before emitting it

[skan]

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

659–660

Comment to myself: plan to eliminate CurByte as possible by using OS.tell() in Part III.

[craig.topper]

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

1401

Nothing looks at the value, the presence of REX is all that is important so we should just return HasREX. I"m not very concerned that we're going to need to know the value of the REX prefix in the future.

[skan]

Address review comments

[craig.topper]

LGTM

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

private:
 
   void emitRegModRMByte(const MCOperand &ModRMReg, unsigned RegOpcodeFld,
                         unsigned &CurByte, raw_ostream &OS) const;
 
   void emitSIBByte(unsigned SS, unsigned Index, unsigned Base,
                    unsigned &CurByte, raw_ostream &OS) const;
 
   void emitMemModRMByte(const MCInst &MI, unsigned Op, unsigned RegOpcodeField,
-                        uint64_t TSFlags, bool Rex, unsigned &CurByte,
+                        uint64_t TSFlags, bool HasREX, unsigned &CurByte,
                         raw_ostream &OS, SmallVectorImpl<MCFixup> &Fixups,
                         const MCSubtargetInfo &STI) const;
 
-  void emitPrefixImpl(unsigned &CurOp, unsigned &CurByte, bool &Rex,
-                      const MCInst &MI, const MCSubtargetInfo &STI,
-                      raw_ostream &OS) const;
+  bool emitPrefixImpl(unsigned &CurOp, unsigned &CurByte, const MCInst &MI,
+                      const MCSubtargetInfo &STI, raw_ostream &OS) const;
 
   void emitVEXOpcodePrefix(unsigned &CurByte, int MemOperand, const MCInst &MI,
                            raw_ostream &OS) const;
 
   void emitSegmentOverridePrefix(unsigned &CurByte, unsigned SegOperand,
                                  const MCInst &MI, raw_ostream &OS) const;
 
   bool emitOpcodePrefix(unsigned &CurByte, int MemOperand, const MCInst &MI,
                         const MCSubtargetInfo &STI, raw_ostream &OS) const;
 
-  uint8_t determineREXPrefix(const MCInst &MI, int MemOperand) const;
+  bool emitREXPrefix(unsigned &CurByte, int MemOperand, const MCInst &MI,
+                     raw_ostream &OS) const;
 };
 
 } // end anonymous namespace
 
 static uint8_t modRMByte(unsigned Mod, unsigned RegOpcode, unsigned RM) {
   assert(Mod < 4 && RegOpcode < 8 && RM < 8 && "ModRM Fields out of range!");
   return RM | (RegOpcode << 3) | (Mod << 6);
 }

 void X86MCCodeEmitter::emitSIBByte(unsigned SS, unsigned Index, unsigned Base,
                                    unsigned &CurByte, raw_ostream &OS) const {
   // SIB byte is in the same format as the modRMByte.
   emitByte(modRMByte(SS, Index, Base), CurByte, OS);
 }
 
 void X86MCCodeEmitter::emitMemModRMByte(const MCInst &MI, unsigned Op,
                                         unsigned RegOpcodeField,
-                                        uint64_t TSFlags, bool Rex,
+                                        uint64_t TSFlags, bool HasREX,
                                         unsigned &CurByte, raw_ostream &OS,
                                         SmallVectorImpl<MCFixup> &Fixups,
                                         const MCSubtargetInfo &STI) const {
   const MCOperand &Disp = MI.getOperand(Op + X86::AddrDisp);
   const MCOperand &Base = MI.getOperand(Op + X86::AddrBaseReg);
   const MCOperand &Scale = MI.getOperand(Op + X86::AddrScaleAmt);
   const MCOperand &IndexReg = MI.getOperand(Op + X86::AddrIndexReg);
   unsigned BaseReg = Base.getReg();
if (BaseReg == X86::RIP ||
     // movq loads are handled with a special relocation form which allows the
     // linker to eliminate some loads for GOT references which end up in the
     // same linkage unit.
     unsigned FixupKind = [=]() {
       switch (Opcode) {
       default:
         return X86::reloc_riprel_4byte;
       case X86::MOV64rm:
-        assert(Rex);
+        assert(HasREX);
         return X86::reloc_riprel_4byte_movq_load;
       case X86::CALL64m:
       case X86::JMP64m:
       case X86::TAILJMPm64:
       case X86::TEST64mr:
       case X86::ADC64rm:
       case X86::ADD64rm:
       case X86::AND64rm:
       case X86::CMP64rm:
       case X86::OR64rm:
       case X86::SBB64rm:
       case X86::SUB64rm:
       case X86::XOR64rm:
-        return Rex ? X86::reloc_riprel_4byte_relax_rex
+        return HasREX ? X86::reloc_riprel_4byte_relax_rex
                       : X86::reloc_riprel_4byte_relax;
       }
     }();
 
     // rip-relative addressing is actually relative to the *next* instruction.
     // Since an immediate can follow the mod/rm byte for an instruction, this
     // means that we need to bias the displacement field of the instruction with
     // the size of the immediate field. If we have this case, add it into the
     // expression to emit.
void X86MCCodeEmitter::emitMemModRMByte(const MCInst &MI, unsigned Op,
   if (ForceDisp8)
     emitImmediate(Disp, MI.getLoc(), 1, FK_Data_1, CurByte, OS, Fixups,
                   ImmOffset);
   else if (ForceDisp32 || Disp.getImm() != 0)
     emitImmediate(Disp, MI.getLoc(), 4, MCFixupKind(X86::reloc_signed_4byte),
                   CurByte, OS, Fixups);
 }
 
-void X86MCCodeEmitter::emitPrefixImpl(unsigned &CurOp, unsigned &CurByte,
-                                      bool &Rex, const MCInst &MI,
+/// Emit all instruction prefixes.
+///
+/// \returns true if REX prefix is used, otherwise returns false.
+bool X86MCCodeEmitter::emitPrefixImpl(unsigned &CurOp, unsigned &CurByte,
+                                      const MCInst &MI,
                                       const MCSubtargetInfo &STI,
                                       raw_ostream &OS) const {
   uint64_t TSFlags = MCII.get(MI.getOpcode()).TSFlags;
   // Determine where the memory operand starts, if present.
   int MemoryOperand = X86II::getMemoryOperandNo(TSFlags);
   // Emit segment override opcode prefix as needed.
   if (MemoryOperand != -1) {
     MemoryOperand += CurOp;
     emitSegmentOverridePrefix(CurByte, MemoryOperand + X86::AddrSegmentReg, MI,
                               OS);
   }
 
if ((STI.hasFeature(X86::Mode16Bit) && AdSize == X86II::AdSize32) ||
     NeedAddressOverride = !is16BitMemOperand(MI, MemoryOperand, STI);
   }
 
   if (NeedAddressOverride)
     emitByte(0x67, CurByte, OS);
 
   // Encoding type for this instruction.
   uint64_t Encoding = TSFlags & X86II::EncodingMask;
-  if (Encoding == 0)
-    Rex = emitOpcodePrefix(CurByte, MemoryOperand, MI, STI, OS);
-  else
+  bool HasREX = false;
+  if (Encoding)
     emitVEXOpcodePrefix(CurByte, MemoryOperand, MI, OS);
+  else
+    HasREX = emitOpcodePrefix(CurByte, MemoryOperand, MI, STI, OS);
 
   uint64_t Form = TSFlags & X86II::FormMask;
   switch (Form) {
   default:
     break;
   case X86II::RawFrmDstSrc: {
     unsigned siReg = MI.getOperand(1).getReg();
     assert(((siReg == X86::SI && MI.getOperand(0).getReg() == X86::DI) ||
case X86II::RawFrmDst: {
     break;
   }
   case X86II::RawFrmMemOffs: {
     // Emit segment override opcode prefix as needed.
     emitSegmentOverridePrefix(CurByte, 1, MI, OS);
     break;
   }
   }
+
+  return HasREX;
 }
 
 /// AVX instructions are encoded using a opcode prefix called VEX.
 void X86MCCodeEmitter::emitVEXOpcodePrefix(unsigned &CurByte, int MemOperand,
                                            const MCInst &MI,
                                            raw_ostream &OS) const {
   const MCInstrDesc &Desc = MCII.get(MI.getOpcode());
   uint64_t TSFlags = Desc.TSFlags;
if (EncodeRC)
                CurByte, OS);
     else
       emitByte((EVEX_z << 7) | (EVEX_L2 << 6) | (VEX_L << 5) | (EVEX_b << 4) |
                    (EVEX_V2 << 3) | EVEX_aaa,
                CurByte, OS);
   }
 }
 
-/// Determine if the MCInst has to be encoded with a X86-64 REX prefix which
-/// specifies 1) 64-bit instructions, 2) non-default operand size, and 3) use
-/// of X86-64 extended registers.
-uint8_t X86MCCodeEmitter::determineREXPrefix(const MCInst &MI,
-                                             int MemOperand) const {
+/// Emit REX prefix which specifies
+///   1) 64-bit instructions,
+///   2) non-default operand size, and
+///   3) use of X86-64 extended registers.
+///
+/// \returns true if REX prefix is used, otherwise returns false.
+bool X86MCCodeEmitter::emitREXPrefix(unsigned &CurByte, int MemOperand,
+                                     const MCInst &MI, raw_ostream &OS) const {
   uint8_t REX = 0;
   bool UsesHighByteReg = false;
 
   const MCInstrDesc &Desc = MCII.get(MI.getOpcode());
   uint64_t TSFlags = Desc.TSFlags;
 
   if (TSFlags & X86II::REX_W)
     REX |= 1 << 3; // set REX.W
bool X86MCCodeEmitter::emitREXPrefix(unsigned &CurByte, int MemOperand,
   case X86II::MRM7r:
     REX |= isREXExtendedReg(MI, CurOp++) << 0; // REX.B
     break;
   }
   if (REX && UsesHighByteReg)
     report_fatal_error(
         "Cannot encode high byte register in REX-prefixed instruction");
 
-  return REX;
+  if (REX)
+    emitByte(0x40 | REX, CurByte, OS);
+
+  return REX != 0;
 }
 
 /// Emit segment override opcode prefix as needed.
 void X86MCCodeEmitter::emitSegmentOverridePrefix(unsigned &CurByte,
                                                  unsigned SegOperand,
                                                  const MCInst &MI,
                                                  raw_ostream &OS) const {
   // Check for explicit segment override on memory operand.
   if (unsigned Reg = MI.getOperand(SegOperand).getReg())
     emitByte(X86::getSegmentOverridePrefixForReg(Reg), CurByte, OS);
 }
 
 /// Emit all instruction prefixes prior to the opcode.
 ///
 /// \param MemOperand the operand # of the start of a memory operand if present.
 /// If not present, it is -1.
 ///
-/// \returns true if a REX prefix was used.
+/// \returns true if REX prefix is used, otherwise returns false.
 bool X86MCCodeEmitter::emitOpcodePrefix(unsigned &CurByte, int MemOperand,
                                         const MCInst &MI,
                                         const MCSubtargetInfo &STI,
                                         raw_ostream &OS) const {
   const MCInstrDesc &Desc = MCII.get(MI.getOpcode());
   uint64_t TSFlags = Desc.TSFlags;
 
-  bool Ret = false;
   // Emit the operand size opcode prefix as needed.
   if ((TSFlags & X86II::OpSizeMask) ==
       (STI.hasFeature(X86::Mode16Bit) ? X86II::OpSize32 : X86II::OpSize16))
     emitByte(0x66, CurByte, OS);
 
   // Emit the LOCK opcode prefix.
   if (TSFlags & X86II::LOCK || MI.getFlags() & X86::IP_HAS_LOCK)
     emitByte(0xF0, CurByte, OS);
case X86II::XS: // F3
     emitByte(0xF3, CurByte, OS);
     break;
   case X86II::XD: // F2
     emitByte(0xF2, CurByte, OS);
     break;
   }
 
   // Handle REX prefix.
-  // FIXME: Can this come before F2 etc to simplify emission?
-  if (STI.hasFeature(X86::Mode64Bit)) {
-    if (uint8_t REX = determineREXPrefix(MI, MemOperand)) {
-      emitByte(0x40 | REX, CurByte, OS);
-      Ret = true;
-    }
-  } else {
-    assert(!(TSFlags & X86II::REX_W) && "REX.W requires 64bit mode.");
-  }
+  assert((STI.hasFeature(X86::Mode64Bit) || !(TSFlags & X86II::REX_W)) &&
+         "REX.W requires 64bit mode.");
+  bool HasREX = STI.hasFeature(X86::Mode64Bit)
+                    ? emitREXPrefix(CurByte, MemOperand, MI, OS)
+                    : false;
 
   // 0x0F escape code must be emitted just before the opcode.
   switch (TSFlags & X86II::OpMapMask) {
   case X86II::TB:        // Two-byte opcode map
   case X86II::T8:        // 0F 38
   case X86II::TA:        // 0F 3A
   case X86II::ThreeDNow: // 0F 0F, second 0F emitted by caller.
     emitByte(0x0F, CurByte, OS);
     break;
   }
 
   switch (TSFlags & X86II::OpMapMask) {
   case X86II::T8: // 0F 38
     emitByte(0x38, CurByte, OS);
     break;
   case X86II::TA: // 0F 3A
     emitByte(0x3A, CurByte, OS);
     break;
   }
-  return Ret;
+
+  return HasREX;
 }
 
 void X86MCCodeEmitter::emitPrefix(const MCInst &MI, raw_ostream &OS,
                                   const MCSubtargetInfo &STI) const {
   unsigned Opcode = MI.getOpcode();
   const MCInstrDesc &Desc = MCII.get(Opcode);
   uint64_t TSFlags = Desc.TSFlags;
 
   // Pseudo instructions don't get encoded.
   if (X86II::isPseudo(TSFlags))
     return;
 
   unsigned CurOp = X86II::getOperandBias(Desc);
 
   // Keep track of the current byte being emitted.
   unsigned CurByte = 0;
 
-  bool Rex = false;
-  emitPrefixImpl(CurOp, CurByte, Rex, MI, STI, OS);
+  emitPrefixImpl(CurOp, CurByte, MI, STI, OS);
 }
 
 void X86MCCodeEmitter::encodeInstruction(const MCInst &MI, raw_ostream &OS,
                                          SmallVectorImpl<MCFixup> &Fixups,
                                          const MCSubtargetInfo &STI) const {
   unsigned Opcode = MI.getOpcode();
   const MCInstrDesc &Desc = MCII.get(Opcode);
   uint64_t TSFlags = Desc.TSFlags;
 
   // Pseudo instructions don't get encoded.
   if (X86II::isPseudo(TSFlags))
     return;
 
   unsigned NumOps = Desc.getNumOperands();
   unsigned CurOp = X86II::getOperandBias(Desc);
 
   // Keep track of the current byte being emitted.
   unsigned CurByte = 0;
 
-  bool Rex = false;
-  emitPrefixImpl(CurOp, CurByte, Rex, MI, STI, OS);
+  bool HasREX = emitPrefixImpl(CurOp, CurByte, MI, STI, OS);
 
   // It uses the VEX.VVVV field?
   bool HasVEX_4V = TSFlags & X86II::VEX_4V;
   bool HasVEX_I8Reg = (TSFlags & X86II::ImmMask) == X86II::Imm8Reg;
 
   // It uses the EVEX.aaa field?
   bool HasEVEX_K = TSFlags & X86II::EVEX_K;
   bool HasEVEX_RC = TSFlags & X86II::EVEX_RC;
case X86II::MRMDestMem: {
 
     if (HasEVEX_K) // Skip writemask
       ++SrcRegNum;
 
     if (HasVEX_4V) // Skip 1st src (which is encoded in VEX_VVVV)
       ++SrcRegNum;
 
     emitMemModRMByte(MI, CurOp, getX86RegNum(MI.getOperand(SrcRegNum)), TSFlags,
-                     Rex, CurByte, OS, Fixups, STI);
+                     HasREX, CurByte, OS, Fixups, STI);
     CurOp = SrcRegNum + 1;
     break;
   }
   case X86II::MRMSrcReg: {
     emitByte(BaseOpcode, CurByte, OS);
     unsigned SrcRegNum = CurOp + 1;
 
     if (HasEVEX_K) // Skip writemask
if (HasEVEX_K) // Skip writemask
       ++FirstMemOp;
 
     if (HasVEX_4V)
       ++FirstMemOp; // Skip the register source (which is encoded in VEX_VVVV).
 
     emitByte(BaseOpcode, CurByte, OS);
 
     emitMemModRMByte(MI, FirstMemOp, getX86RegNum(MI.getOperand(CurOp)),
-                     TSFlags, Rex, CurByte, OS, Fixups, STI);
+                     TSFlags, HasREX, CurByte, OS, Fixups, STI);
     CurOp = FirstMemOp + X86::AddrNumOperands;
     if (HasVEX_I8Reg)
       I8RegNum = getX86RegEncoding(MI, CurOp++);
     break;
   }
   case X86II::MRMSrcMem4VOp3: {
     unsigned FirstMemOp = CurOp + 1;
 
     emitByte(BaseOpcode, CurByte, OS);
 
     emitMemModRMByte(MI, FirstMemOp, getX86RegNum(MI.getOperand(CurOp)),
-                     TSFlags, Rex, CurByte, OS, Fixups, STI);
+                     TSFlags, HasREX, CurByte, OS, Fixups, STI);
     CurOp = FirstMemOp + X86::AddrNumOperands;
     ++CurOp; // Encoded in VEX.VVVV.
     break;
   }
   case X86II::MRMSrcMemOp4: {
     unsigned FirstMemOp = CurOp + 1;
 
     ++FirstMemOp; // Skip the register source (which is encoded in VEX_VVVV).
 
     // Capture second register source (encoded in Imm[7:4])
     assert(HasVEX_I8Reg && "MRMSrcRegOp4 should imply VEX_I8Reg");
     I8RegNum = getX86RegEncoding(MI, FirstMemOp++);
 
     emitByte(BaseOpcode, CurByte, OS);
 
     emitMemModRMByte(MI, FirstMemOp, getX86RegNum(MI.getOperand(CurOp)),
-                     TSFlags, Rex, CurByte, OS, Fixups, STI);
+                     TSFlags, HasREX, CurByte, OS, Fixups, STI);
     CurOp = FirstMemOp + X86::AddrNumOperands;
     break;
   }
   case X86II::MRMSrcMemCC: {
     unsigned RegOp = CurOp++;
     unsigned FirstMemOp = CurOp;
     CurOp = FirstMemOp + X86::AddrNumOperands;
 
     unsigned CC = MI.getOperand(CurOp++).getImm();
     emitByte(BaseOpcode + CC, CurByte, OS);
 
     emitMemModRMByte(MI, FirstMemOp, getX86RegNum(MI.getOperand(RegOp)),
-                     TSFlags, Rex, CurByte, OS, Fixups, STI);
+                     TSFlags, HasREX, CurByte, OS, Fixups, STI);
     break;
   }
 
   case X86II::MRMXrCC: {
     unsigned RegOp = CurOp++;
 
     unsigned CC = MI.getOperand(CurOp++).getImm();
     emitByte(BaseOpcode + CC, CurByte, OS);
void X86MCCodeEmitter::encodeInstruction(const MCInst &MI, raw_ostream &OS,
 
   case X86II::MRMXmCC: {
     unsigned FirstMemOp = CurOp;
     CurOp = FirstMemOp + X86::AddrNumOperands;
 
     unsigned CC = MI.getOperand(CurOp++).getImm();
     emitByte(BaseOpcode + CC, CurByte, OS);
 
-    emitMemModRMByte(MI, FirstMemOp, 0, TSFlags, Rex, CurByte, OS, Fixups, STI);
+    emitMemModRMByte(MI, FirstMemOp, 0, TSFlags, HasREX, CurByte, OS, Fixups,
+                     STI);
     break;
   }
 
   case X86II::MRMXm:
   case X86II::MRM0m:
   case X86II::MRM1m:
   case X86II::MRM2m:
   case X86II::MRM3m:
   case X86II::MRM4m:
   case X86II::MRM5m:
   case X86II::MRM6m:
   case X86II::MRM7m:
     if (HasVEX_4V) // Skip the register dst (which is encoded in VEX_VVVV).
       ++CurOp;
     if (HasEVEX_K) // Skip writemask
       ++CurOp;
     emitByte(BaseOpcode, CurByte, OS);
     emitMemModRMByte(MI, CurOp,
                      (Form == X86II::MRMXm) ? 0 : Form - X86II::MRM0m, TSFlags,
-                     Rex, CurByte, OS, Fixups, STI);
+                     HasREX, CurByte, OS, Fixups, STI);
     CurOp += X86::AddrNumOperands;
     break;
 
   case X86II::MRM_C0:
   case X86II::MRM_C1:
   case X86II::MRM_C2:
   case X86II::MRM_C3:
   case X86II::MRM_C4: