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

Tue Feb 7 01:30:11 PST 2023

skan added inline comments.

================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp:1257
+/// \returns the used prefix (REX or None).
+PrefixKind X86MCCodeEmitter::emitOpcodePrefix(int MemOperand, const MCInst &MI,
                                         const MCSubtargetInfo &STI,
----------------
skan wrote:
> craig.topper wrote:
> > I don't know that it makes sense to make returning the bool for `HasREX` worse by spreading it to more functions. None of the other prefixes are needed and I'm not sure there's sufficient evidence they ever will be. And if they were needed would what type of prefix be enough information?
> > 
> > A `bool &HasREX` passed to emitOpcodePrefix and emitREXPrefix feels cleaner to me. I never liked the returned bool for this.
> I know a little about it. When X86 introduces a bunch of new instructions, it usually extends the prefix. For example,
> when moved from ia32 to ia32e, REX was defined. Similarly, when we moved from SSE to AVX, VEX prefix was defined.
> 
> And when we introduce new instructions, a new relocation may be needed. Here is the example for REX
> https://groups.google.com/g/x86-64-abi/c/n9AWHogmVY0
> 
> Linker can do relocation optimization based on the relocation. From my understanding, if no new relocation were not added for the new instructions, the optimization could be done in an incorrect way silently.
> 
> Only a bool `HasREX` was defined here b/c the current interested instructions are only REX-encoded. 
> But we should allow more possibility here.   And returning a enum here is almost as cheap as bool.
> 
>   
Got your idea. Let me to pass the enum by reference rather than a return value.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143471