[PATCH] D78180: [X86][MC][NFC] Reduce the parameters of functions in X86MCCodeEmitter(Part I)

Wed Apr 15 14:19:31 PDT 2020

craig.topper accepted this revision.
craig.topper added a comment.
This revision is now accepted and ready to land.

This patch LGTM. The comments are all for future work.

================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp:644
   // Emit segment override opcode prefix as needed.
   if (MemoryOperand >= 0)
     emitSegmentOverridePrefix(CurByte, MemoryOperand + X86::AddrSegmentReg, MI,
----------------
While you're cleaning up things in this file? Can you merge this if with the "MemoryOperand != -1" above. The only negative value returned by getMemoryOperandNo is -1 so these ifs are the same. Separate patch.

================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp:656
   // Emit the address size opcode prefix as needed.
   bool need_address_override;
   uint64_t AdSize = TSFlags & X86II::AdSizeMask;
----------------
Also fix this to be CamelCase not snake_case.

================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp:1309-1310

   // Handle REX prefix.
   // FIXME: Can this come before F2 etc to simplify emission?
   if (STI.hasFeature(X86::Mode64Bit)) {
----------------
skan wrote:
> Comment to myself: Plan to fix this "FIXME" in Part2
Pretty sure REX prefix has to come after 0xF2 so I don't think that can be fixed.

================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp:1312
   if (STI.hasFeature(X86::Mode64Bit)) {
-    if (uint8_t REX = determineREXPrefix(MI, TSFlags, MemOperand, Desc)) {
+    if (uint8_t REX = determineREXPrefix(MI, MemOperand)) {
       emitByte(0x40 | REX, CurByte, OS);
----------------
Should we just make determineREXPrefix be emitREXPrefix and move the emitByte into it. Its more arguments, but more consistent.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78180/new/

https://reviews.llvm.org/D78180