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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp:748
   switch (Size) {
   default: return X86::NoRegister;
   case 8:
----------------
Why do we allow invalid sizes?


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp:873
     switch (Reg.id()) {
     default: return 0;
     case X86::AH: case X86::AL: case X86::AX: case X86::EAX: case X86::RAX:
----------------
This should be NoRegister


================
Comment at: llvm/lib/Target/X86/X86AsmPrinter.cpp:545
   case 'b': // Print QImode register
     Reg = getX86SubSuperRegister(Reg, 8);
     break;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142834