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

[Kan Shengchen via Phabricator via llvm-commits]

skan added inline comments.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp:774-776
+PrefixKind X86MCCodeEmitter::emitPrefixImpl(unsigned &CurOp, const MCInst &MI,
                                       const MCSubtargetInfo &STI,
                                       raw_ostream &OS) const {
----------------
pengfei wrote:
> Format.
Will do


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp:855
 
-  uint64_t Encoding = TSFlags & X86II::EncodingMask;
+  X86OpcodePrefix Prefix(*Ctx.getRegisterInfo());
+  switch (TSFlags & X86II::EncodingMask) {
----------------
pengfei wrote:
> Should be better to move below logic into `X86OpcodePrefix `? Then we can simplify the function to:
> 
> ```
>   X86OpcodePrefix Prefix(*Ctx.getRegisterInfo(), TSFlags);
>   Prefix.emit(OS);
>   return Prefix.getKind();
> ```
We can not b/c we still need to read the operands the instruction before the final emit.
We shouldn't b/c the first principle mentioned in the summary:

"Make code clearer by separating the logic of setting bits from the logic of how a prefix is encoded"


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp:860
+  case X86II::XOP:
+    Prefix.setLowerBound(XOP);
+    break;
----------------
pengfei wrote:
> Should be better to use `setEncoding`?
I think `setLowerBound` is more clear than `setEncoding` here b/c VEX and REX are determined at the last stage. e.g
`Prefix.setEncoding(VEX2)` here is counterintuitive.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143471