[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:1257
+/// \returns the used prefix (REX or None).
+PrefixKind X86MCCodeEmitter::emitOpcodePrefix(int MemOperand, const MCInst &MI,
                                         const MCSubtargetInfo &STI,
----------------
craig.topper wrote:
> craig.topper wrote:
> > skan wrote:
> > > 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.
> > REX is older than the VEX, XOP, EVEX prefixes so it doesn’t seem common to need the new relocation.
> > 
> > It was cheaper to compute I’d say we could determine if it has REX in the fixup code but we have to inspect multiple operands.
> > 
> > If we need to know about a prefix for a future location that one might be cheaper to compute.
> > 
> > This feels like we’re trying to solve a future problem that might never exist. Is it really worth it?
> *relocation
I just gave a try.  But passing the `enum` as argument increases the length of arglists for four functions: `emitREXPrefix`, `emitVEXOpcodePrefix`, `emitOpcodePrefix` and `emitPrefixImpl` longer, which made me nervous. I don't even know which order I should use for the parameters for the functions. 

I remembered that we once refined the code in this file to reduce the parameters, so I prefer the "return value" version. 
AFAICS,  whether the prefix is REX, VEX, or EVEX should be enough information for the other code in `emitInstructiion`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143471