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

[skan]

Summary

MCRegister getX86SubSuperRegister*(MCRegister Reg, unsigned Size,
                                  bool High = false);

A strange behavior of the functions getX86SubSuperRegister* was
introduced by llvm-svn:145579: The returned register may not
match the parameters when a 8-bit high register is required.

And llvm-svn: 175762 refined the code and dropped the comments, then we
knew nothing happened there from the code :-(

These two functions are only called with Size=8 and High=true in two places.
One is in X86FixupBWInsts.cpp for liveness of registers and the other is in
X86AsmPrinter.cpp for inline asm.

For the first one, we provide an alternative in this patch.
For the second one, the strange behaviour caused a bug that an erorr was not reported for mismatched modifier.

void f() {
  char x;
  asm volatile ("mov %%ah, %h0" :"=r"(x)::"%eax", "%ebx", "%ecx", "%edx", "edi", "esi");
}

$ gcc -S test.c

error: extended registers have no high halves 

$ clang -S test.c

no error

so we fix the bug in this patch.

getX86SubSuperRegister is just a wrapper of getX86SubSuperRegisterOrZero with a assert.
I belive we should remove the latter.

[skan]

Fix typo in comments

[pengfei]

The change doesn't seem correct. It seems the code is used for special case, see https://github.com/llvm/llvm-project/commit/e93249befaa2383cc34c27771265123090cda085

[skan]

@pengfei I noticed and mentioned this commit in the summary. I think the special purpose is only for AH, DH, CH and DH b/c they are the only 8-bit high registers that are not artifical. For others, X86 backend does not have subregister liveness tracking enabled, so either 16 or 64 can work.
There is no reason to make a difference for SI, DI, BP and SP.

[skan]

In D142834#4088767, @skan wrote:

@pengfei I noticed and mentioned this commit in the summary. I think the special purpose is only for AH, DH, CH and DH b/c they are the only 8-bit high registers that are not artifical. For others, X86 backend does not have subregister liveness tracking enabled, so either 16 or 64 can work.
There is no reason to make a difference for SI, DI, BP and SP.

I checked that there is no LIT test fail no matter we use getX86SubSuperRegisterOrZero(Reg, 16) or getX86SubSuperRegisterOrZero(Reg, 64) here.

[skan]

Not use report_fatal_error b/c it's unrelated to user input

[pengfei]

In D142834#4088776, @skan wrote:

Not use report_fatal_error b/c it's unrelated to user input

There will be difference with dedicated cases, e.g.: https://godbolt.org/z/oca6bj4s3

[skan]

In D142834#4088826, @pengfei wrote:

In D142834#4088776, @skan wrote:

Not use report_fatal_error b/c it's unrelated to user input

There will be difference with dedicated cases, e.g.: https://godbolt.org/z/oca6bj4s3

It is an impossible case. We can only access 8-bit high register for register a, b, c d in inline asm.

void f() {
  char x;
  asm volatile ("mov %h0, %h0" :"=r"(x)::);
  asm volatile ("mov %h0, %h0" :"=r"(x)::"%eax", "%ebx", "%ecx", "%edx", "edi", "esi");
}

bash$ gcc -S 1.c -o -
        .file   "1.c"
        .text
        .globl  f
        .type   f, @function
f:
.LFB0:
        .cfi_startproc
        pushq   %rbp
        .cfi_def_cfa_offset 16
        .cfi_offset 6, -16
        movq    %rsp, %rbp
        .cfi_def_cfa_register 6
        pushq   %rbx
        .cfi_offset 3, -24
#APP
# 3 "1.c" 1
        mov %ah, %ah
# 0 "" 2
#NO_APP
        movb    %al, -9(%rbp)
#APP
# 4 "1.c" 1
1.c: In function ‘f’:
1.c:5:1: error: extended registers have no high halves

[pengfei]

In D142834#4088861, @skan wrote:

In D142834#4088826, @pengfei wrote:

In D142834#4088776, @skan wrote:

Not use report_fatal_error b/c it's unrelated to user input

There will be difference with dedicated cases, e.g.: https://godbolt.org/z/oca6bj4s3

It is an impossible case. We can only access 8-bit high register for register a, b, c d in inline asm.

void f() {
  char x;
  asm volatile ("mov %h0, %h0" :"=r"(x)::);
  asm volatile ("mov %h0, %h0" :"=r"(x)::"%eax", "%ebx", "%ecx", "%edx", "edi", "esi");
}

bash$ gcc -S 1.c -o -
        .file   "1.c"
        .text
        .globl  f
        .type   f, @function
f:
.LFB0:
        .cfi_startproc
        pushq   %rbp
        .cfi_def_cfa_offset 16
        .cfi_offset 6, -16
        movq    %rsp, %rbp
        .cfi_def_cfa_register 6
        pushq   %rbx
        .cfi_offset 3, -24
#APP
# 3 "1.c" 1
        mov %ah, %ah
# 0 "" 2
#NO_APP
        movb    %al, -9(%rbp)
#APP
# 4 "1.c" 1
1.c: In function ‘f’:
1.c:5:1: error: extended registers have no high halves

I'm not sure if there's special requirment in LLVM. If we want to match with gcc, should we just need to replace return getX86SubSuperRegisterOrZero(Reg, 64); with return X86::NoRegister?

[skan]

In D142834#4089368, @pengfei wrote:

In D142834#4088861, @skan wrote:

In D142834#4088826, @pengfei wrote:

In D142834#4088776, @skan wrote:

Not use report_fatal_error b/c it's unrelated to user input

There will be difference with dedicated cases, e.g.: https://godbolt.org/z/oca6bj4s3

It is an impossible case. We can only access 8-bit high register for register a, b, c d in inline asm.

void f() {
  char x;
  asm volatile ("mov %h0, %h0" :"=r"(x)::);
  asm volatile ("mov %h0, %h0" :"=r"(x)::"%eax", "%ebx", "%ecx", "%edx", "edi", "esi");
}

bash$ gcc -S 1.c -o -
        .file   "1.c"
        .text
        .globl  f
        .type   f, @function
f:
.LFB0:
        .cfi_startproc
        pushq   %rbp
        .cfi_def_cfa_offset 16
        .cfi_offset 6, -16
        movq    %rsp, %rbp
        .cfi_def_cfa_register 6
        pushq   %rbx
        .cfi_offset 3, -24
#APP
# 3 "1.c" 1
        mov %ah, %ah
# 0 "" 2
#NO_APP
        movb    %al, -9(%rbp)
#APP
# 4 "1.c" 1
1.c: In function ‘f’:
1.c:5:1: error: extended registers have no high halves

I'm not sure if there's special requirment in LLVM. If we want to match with gcc, should we just need to replace return getX86SubSuperRegisterOrZero(Reg, 64); with return X86::NoRegister?

I think so. And it looks like one of getX86SubSuperRegisterOrZero and getX86SubSuperRegister is redundant, let me do some clean work.

[skan]

Remove function getX86SubSuperRegisterOrZero

[craig.topper]

llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp

746

Didn't the old name more accurately describe this function?

llvm/lib/Target/X86/X86AsmPrinter.cpp

545 ↗

(On Diff #493197)

Do we need to assert that Reg is valid? We used to have one.

549 ↗

(On Diff #493197)

Reg.isValid()

llvm/lib/Target/X86/X86FixupBWInsts.cpp

218 ↗

(On Diff #493197)

HighReg.isValid()

llvm/lib/Target/X86/X86ISelLowering.cpp

57580 ↗

(On Diff #493197)

DestReg.isValid()

[skan]

llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp

746

I don't think so

1. X86::NoRegister is just a invalid value and it doesn't have to be zero
2. When we talk about a API like getXXX, we can expect it to return an invalid value, e.g we use getCondCode rather than getCondCodeOrInvalidCond

llvm/lib/Target/X86/X86AsmPrinter.cpp

545 ↗

(On Diff #493197)

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.

[skan]

Address review comments

[craig.topper]

llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp

748

Why do we allow invalid sizes?

873

This should be NoRegister

llvm/lib/Target/X86/X86AsmPrinter.cpp

545 ↗

(On Diff #493197)

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.

[skan]

llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp

748

I think the previous author was overly conservative here. Let's use a llvm_unreachable here.

llvm/lib/Target/X86/X86AsmPrinter.cpp

545 ↗

(On Diff #493197)

Yes, we lose it in other places too. There is only a few places that need the assertion, let me add it.

[skan]

Address reivew comments

[skan]

llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp

748

Why do we allow invalid sizes?

The test CodeGen/X86/asm-reject-reg-type-mismatch.ll will fail if we use llvm_unreachable here.

; CHECK: error: couldn't allocate output register for constraint '{ax}'
define i128 @blup() {
  %v = tail call i128 asm "", "={ax},0"(i128 0)
  ret i128 %v
}

See more details here https://reviews.llvm.org/D10813

[pengfei]

llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp

748

Is it caused by error message not match?

[skan]

llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp

748

Is it caused by error message not match?

It is caused by the crash due to the llvm_unreachable at line 748.

UNREACHABLE executed at llvm-project/llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp:748!

getX86SubSuperRegister is called with Size 128, and an inline asm error is emitted in SelectionDAGBuilder.cpp when the returned register is not valid.
If we used "llvm_unreachable" at line 748, the llc would crash before emitting an error.

[pengfei]

llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp

748

Will it be solved by adding --crash after not?

[skan]

llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp

748

Yes, we also need to move other tests in asm-reject-reg-type-mismatch.ll into another file

; CHECK: error: couldn't allocate input reg for constraint 'r'
define void @fp80(x86_fp80) {
  tail call void asm sideeffect "", "r"(x86_fp80 %0)
  ret void
}

; CHECK: error: couldn't allocate input reg for constraint 'f'
define void @f_constraint_i128(ptr %0) {
  %2 = load i128, ptr %0, align 16
  tail call void asm sideeffect "", "f"(i128 %2)
  ret void
}

b/c when llc crashes, the remaining file will not be processed. But, should we do such thing?

I think we can just remove the function blup and the check in this test b/c it is impossible case. Does it sound good to you?

[skan]

Use llvm_unreachable for illeagl register size

[craig.topper]

llvm/test/CodeGen/X86/asm-reject-reg-type-mismatch.ll

4 ↗

(On Diff #493478)

Is it not possible to do this with __int128 in C?

[craig.topper]

llvm/test/CodeGen/X86/asm-reject-reg-type-mismatch.ll

4 ↗

(On Diff #493478)

void foo() {
    __int128 a;
    asm volatile ("" : "+a"(a));
}

[skan]

llvm/test/CodeGen/X86/asm-reject-reg-type-mismatch.ll

4 ↗

(On Diff #493478)

void foo() {
    __int128 a;
    asm volatile ("" : "+a"(a));
}

Good example!

[skan]

Address review comments: add a test back

[skan]

@craig.topper I keep the llvm_unreachable by using a pre-check in X86ISelLowering.cpp. It's the only place I found the getX86SubSuperRegister is called with illeagl size.

[pengfei]

LGTM.

llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.h

 
 /// Construct an X86 ELF object writer.
 std::unique_ptr<MCObjectTargetWriter>
 createX86ELFObjectWriter(bool IsELF64, uint8_t OSABI, uint16_t EMachine);
 /// Construct an X86 Win COFF object writer.
 std::unique_ptr<MCObjectTargetWriter>
 createX86WinCOFFObjectWriter(bool Is64Bit);
 
-/// Returns the sub or super register of a specific X86 register.
-/// e.g. getX86SubSuperRegister(X86::EAX, 16) returns X86::AX.
-/// Aborts on error.
-MCRegister getX86SubSuperRegister(MCRegister, unsigned, bool High=false);
-
-/// Returns the sub or super register of a specific X86 register.
-/// Like getX86SubSuperRegister() but returns 0 on error.
-MCRegister getX86SubSuperRegisterOrZero(MCRegister, unsigned,
+/// \param Reg speicifed register.
+/// \param Size the bit size of returned register.
+/// \param High requires the high register.
+///
+/// \returns the sub or super register of a specific X86 register, if the
+/// 8-bit high register does not exist, a 16-bit register will be returned.
+/// Abort when no register is found.
+MCRegister getX86SubSuperRegister(MCRegister Reg, unsigned Size,
+                                  bool High = false);
+
+/// \param Reg speicifed register.
+/// \param Size the bit size of returned register.
+/// \param High requires the high register.
+///
+/// \returns the sub or super register of a specific X86 register, if the
+/// 8-bit high register does not exist, a 16-bit register will be returned.
+MCRegister getX86SubSuperRegisterOrZero(MCRegister Reg, unsigned Size,
                                         bool High = false);
 
 } // End llvm namespace
 
 
 // Defines symbolic names for X86 registers.  This defines a mapping from
 // register name to register number.
 //

llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp

extern "C" LLVM_EXTERNAL_VISIBILITY void LLVMInitializeX86TargetMC() {
   // Register the asm backend.
   TargetRegistry::RegisterMCAsmBackend(getTheX86_32Target(),
                                        createX86_32AsmBackend);
   TargetRegistry::RegisterMCAsmBackend(getTheX86_64Target(),
                                        createX86_64AsmBackend);
 }
 
 MCRegister llvm::getX86SubSuperRegisterOrZero(MCRegister Reg, unsigned Size,
                                               bool High) {
   switch (Size) {
   default: return X86::NoRegister;
   case 8:
     if (High) {
       switch (Reg.id()) {
-      default: return getX86SubSuperRegisterOrZero(Reg, 64);
-      case X86::SIL: case X86::SI: case X86::ESI: case X86::RSI:
-        return X86::SI;
-      case X86::DIL: case X86::DI: case X86::EDI: case X86::RDI:
-        return X86::DI;
-      case X86::BPL: case X86::BP: case X86::EBP: case X86::RBP:
-        return X86::BP;
-      case X86::SPL: case X86::SP: case X86::ESP: case X86::RSP:
-        return X86::SP;
+      default: return getX86SubSuperRegisterOrZero(Reg, 16);
       case X86::AH: case X86::AL: case X86::AX: case X86::EAX: case X86::RAX:
         return X86::AH;
       case X86::DH: case X86::DL: case X86::DX: case X86::EDX: case X86::RDX:
         return X86::DH;
       case X86::CH: case X86::CL: case X86::CX: case X86::ECX: case X86::RCX:
         return X86::CH;
       case X86::BH: case X86::BL: case X86::BX: case X86::EBX: case X86::RBX:
         return X86::BH;
case X86::R13B: case X86::R13W: case X86::R13D: case X86::R13:
       return X86::R13D;
     case X86::R14B: case X86::R14W: case X86::R14D: case X86::R14:
       return X86::R14D;
     case X86::R15B: case X86::R15W: case X86::R15D: case X86::R15:
       return X86::R15D;
     }
   case 64:
     switch (Reg.id()) {
     default: return 0;
     case X86::AH: case X86::AL: case X86::AX: case X86::EAX: case X86::RAX:
       return X86::RAX;
     case X86::DH: case X86::DL: case X86::DX: case X86::EDX: case X86::RDX:
       return X86::RDX;
     case X86::CH: case X86::CL: case X86::CX: case X86::ECX: case X86::RCX:
       return X86::RCX;
     case X86::BH: case X86::BL: case X86::BX: case X86::EBX: case X86::RBX:
       return X86::RBX;
case 64:
     case X86::R14B: case X86::R14W: case X86::R14D: case X86::R14:
       return X86::R14;
     case X86::R15B: case X86::R15W: case X86::R15D: case X86::R15:
       return X86::R15;
     }
   }
 }
 
-MCRegister llvm::getX86SubSuperRegister(MCRegister Reg, unsigned Size, bool High) {
+MCRegister llvm::getX86SubSuperRegister(MCRegister Reg, unsigned Size,
+                                        bool High) {
   MCRegister Res = getX86SubSuperRegisterOrZero(Reg, Size, High);
-  assert(Res != X86::NoRegister && "Unexpected register or VT");
+  if (Res == X86::NoRegister)
+    report_fatal_error("Unexpected register or size");
   return Res;
 }
-
-