[PATCH] D143471: [X86][MC][NFC] Refine code in X86MCCodeEmitter.cpp about opcode prefix

Wed Feb 8 22:13:01 PST 2023

skan added a comment.

In D143471#4114625 <https://reviews.llvm.org/D143471#4114625>, @craig.topper wrote:

> Thinking....  What if we took this further and had an "encodeable instruction" object that contains the prefix fields and the modrm fields that we build by walking the operands and format once. Then we would only have 1 switch on format instead of the 3 we have now. Then we use that to emit the prefix, the opcode, and the modrm byte.

This sounds good! Let me give a try.

================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp:140
+  void setR(const MCInst &MI, unsigned OpNum) {
+    R = MRI.getEncodingValue(MI.getOperand(OpNum).getReg()) >> 3 & 1;
+  }
----------------
craig.topper wrote:
> I'm not sure if it makes sense to have this class understand MRI and MI. I kind of think it should only receive the register encoding.
> 
> Especially when you look at funcions like setRR2 that now end up looking up the encoding twice because it nests calls to setR and setR2.
I agree the name `X86OpcodePrefix` doesn't sound like much to understand MI, we should have a "Helper" suffix.

Encoding twice is not a big issue. We can easily extract a function "getX86RegEncoding" to avoid this.

I am thinking your new proposal "encodeable instruction" 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143471