[PATCH] D142834: [X86][MC][bugfix] Report error for mismatched modifier in inline asm and remove function getX86SubSuperRegisterOrZero

[Kan Shengchen via Phabricator via llvm-commits]

skan added inline comments.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp:748
   switch (Size) {
   default: return X86::NoRegister;
   case 8:
----------------
craig.topper wrote:
> Why do we allow invalid sizes?
I think the previous author was overly conservative here. Let's use a `llvm_unreachable` here.


================
Comment at: llvm/lib/Target/X86/X86AsmPrinter.cpp:545
   case 'b': // Print QImode register
     Reg = getX86SubSuperRegister(Reg, 8);
     break;
----------------
craig.topper wrote:
> skan wrote:
> > craig.topper wrote:
> > > Do we need to assert that Reg is valid? We used to have one.
> > https://llvm.org/docs/CodingStandards.html#assert-liberally 
> > 
> > `assert` is disabled for release build.  I belive it is only used to report internal error for LLVM developers and it should be used only when the developer has concern.
> > We have checked the register class of `Reg` at line 536-539, so `Reg` is always valid here obviously.  I think  `assertion` is no need here.
> Did we lose the assertion in other places too? Unfortunately due to the rename, I can't see all callers of the old  getX86SubSuperRegister in the review.
Yes, we lose it in other places too. There is only a few places that need the `assertion`, let me add it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142834