[llvm] 011e4ab - [X86][MC][bugfix] Report error for mismatched modifier in inline asm and remove function getX86SubSuperRegisterOrZero
Shengchen Kan via llvm-commits
llvm-commits at lists.llvm.org
Wed Feb 1 18:09:04 PST 2023
Author: Shengchen Kan
Date: 2023-02-02T10:08:56+08:00
New Revision: 011e4abb498098aed2f8447dd4117d5c4e09c0f0
URL: https://github.com/llvm/llvm-project/commit/011e4abb498098aed2f8447dd4117d5c4e09c0f0
DIFF: https://github.com/llvm/llvm-project/commit/011e4abb498098aed2f8447dd4117d5c4e09c0f0.diff
LOG: [X86][MC][bugfix] Report error for mismatched modifier in inline asm and remove function getX86SubSuperRegisterOrZero
```
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.
Reviewed By: pengfei
Differential Revision: https://reviews.llvm.org/D142834
Added:
llvm/test/CodeGen/X86/asm-modifier-error.ll
Modified:
llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp
llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.h
llvm/lib/Target/X86/X86AsmPrinter.cpp
llvm/lib/Target/X86/X86FixupBWInsts.cpp
llvm/lib/Target/X86/X86FrameLowering.cpp
llvm/lib/Target/X86/X86ISelLowering.cpp
llvm/lib/Target/X86/X86InstrInfo.cpp
llvm/lib/Target/X86/X86MCInstLower.cpp
Removed:
################################################################################
diff --git a/llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp b/llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
index 5409b112e7440..b45be0d842ddc 100644
--- a/llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
+++ b/llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
@@ -3500,8 +3500,8 @@ bool X86AsmParser::ParseInstruction(ParseInstructionInfo &Info, StringRef Name,
Operands[0] = X86Operand::CreateToken(Name, NameLoc);
}
// Select the correct equivalent 16-/32-bit source register.
- unsigned Reg =
- getX86SubSuperRegisterOrZero(Op1.getReg(), is16BitMode() ? 16 : 32);
+ MCRegister Reg =
+ getX86SubSuperRegister(Op1.getReg(), is16BitMode() ? 16 : 32);
Operands[1] = X86Operand::CreateReg(Reg, Loc, Loc);
}
}
diff --git a/llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp b/llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp
index 100e45e72189b..db61aacd992f8 100644
--- a/llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp
+++ b/llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp
@@ -742,22 +742,14 @@ extern "C" LLVM_EXTERNAL_VISIBILITY void LLVMInitializeX86TargetMC() {
createX86_64AsmBackend);
}
-MCRegister llvm::getX86SubSuperRegisterOrZero(MCRegister Reg, unsigned Size,
- bool High) {
+MCRegister llvm::getX86SubSuperRegister(MCRegister Reg, unsigned Size,
+ bool High) {
switch (Size) {
- default: return X86::NoRegister;
+ default: llvm_unreachable("illegal register size");
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 X86::NoRegister;
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:
@@ -878,7 +870,7 @@ MCRegister llvm::getX86SubSuperRegisterOrZero(MCRegister Reg, unsigned Size,
}
case 64:
switch (Reg.id()) {
- default: return 0;
+ default: return X86::NoRegister;
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:
@@ -914,11 +906,3 @@ MCRegister llvm::getX86SubSuperRegisterOrZero(MCRegister Reg, unsigned Size,
}
}
}
-
-MCRegister llvm::getX86SubSuperRegister(MCRegister Reg, unsigned Size, bool High) {
- MCRegister Res = getX86SubSuperRegisterOrZero(Reg, Size, High);
- assert(Res != X86::NoRegister && "Unexpected register or VT");
- return Res;
-}
-
-
diff --git a/llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.h b/llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.h
index d0530bd4d6505..437a7bd6ff6c4 100644
--- a/llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.h
+++ b/llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.h
@@ -135,16 +135,13 @@ createX86ELFObjectWriter(bool IsELF64, uint8_t OSABI, uint16_t EMachine);
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,
- 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.
+MCRegister getX86SubSuperRegister(MCRegister Reg, unsigned Size,
+ bool High = false);
} // End llvm namespace
diff --git a/llvm/lib/Target/X86/X86AsmPrinter.cpp b/llvm/lib/Target/X86/X86AsmPrinter.cpp
index 88bc4b072ac8b..4e725d7829809 100644
--- a/llvm/lib/Target/X86/X86AsmPrinter.cpp
+++ b/llvm/lib/Target/X86/X86AsmPrinter.cpp
@@ -546,6 +546,8 @@ static bool printAsmMRegister(const X86AsmPrinter &P, const MachineOperand &MO,
break;
case 'h': // Print QImode high register
Reg = getX86SubSuperRegister(Reg, 8, true);
+ if (!Reg.isValid())
+ return true;
break;
case 'w': // Print HImode register
Reg = getX86SubSuperRegister(Reg, 16);
diff --git a/llvm/lib/Target/X86/X86FixupBWInsts.cpp b/llvm/lib/Target/X86/X86FixupBWInsts.cpp
index db69234161774..0e8f8c9bc6d96 100644
--- a/llvm/lib/Target/X86/X86FixupBWInsts.cpp
+++ b/llvm/lib/Target/X86/X86FixupBWInsts.cpp
@@ -193,6 +193,7 @@ bool FixupBWInstPass::getSuperRegDestIfDead(MachineInstr *OrigMI,
const X86RegisterInfo *TRI = &TII->getRegisterInfo();
Register OrigDestReg = OrigMI->getOperand(0).getReg();
SuperDestReg = getX86SubSuperRegister(OrigDestReg, 32);
+ assert(SuperDestReg.isValid() && "Invalid Operand");
const auto SubRegIdx = TRI->getSubRegIndex(SuperDestReg, OrigDestReg);
@@ -213,9 +214,9 @@ bool FixupBWInstPass::getSuperRegDestIfDead(MachineInstr *OrigMI,
// If the original destination register was the low 8-bit subregister and
// we also need to check the 16-bit subregister and the high 8-bit
// subregister.
+ MCRegister HighReg = getX86SubSuperRegister(SuperDestReg, 8, /*High=*/true);
if (!LiveRegs.contains(getX86SubSuperRegister(OrigDestReg, 16)) &&
- !LiveRegs.contains(getX86SubSuperRegister(SuperDestReg, 8,
- /*High=*/true)))
+ (!HighReg.isValid() || !LiveRegs.contains(HighReg)))
return true;
// Otherwise, we have a little more checking to do.
}
@@ -327,6 +328,7 @@ MachineInstr *FixupBWInstPass::tryReplaceCopy(MachineInstr *MI) const {
return nullptr;
Register NewSrcReg = getX86SubSuperRegister(OldSrc.getReg(), 32);
+ assert(NewSrcReg.isValid() && "Invalid Operand");
// This is only correct if we access the same subregister index: otherwise,
// we could try to replace "movb %ah, %al" with "movl %eax, %eax".
diff --git a/llvm/lib/Target/X86/X86FrameLowering.cpp b/llvm/lib/Target/X86/X86FrameLowering.cpp
index 189612108adfe..de33699810a05 100644
--- a/llvm/lib/Target/X86/X86FrameLowering.cpp
+++ b/llvm/lib/Target/X86/X86FrameLowering.cpp
@@ -528,7 +528,7 @@ void X86FrameLowering::emitZeroCallUsedRegs(BitVector RegsToZero,
BitVector GPRsToZero(TRI->getNumRegs());
for (MCRegister Reg : RegsToZero.set_bits())
if (TRI->isGeneralPurposeRegister(MF, Reg)) {
- GPRsToZero.set(getX86SubSuperRegisterOrZero(Reg, 32));
+ GPRsToZero.set(getX86SubSuperRegister(Reg, 32));
RegsToZero.reset(Reg);
}
diff --git a/llvm/lib/Target/X86/X86ISelLowering.cpp b/llvm/lib/Target/X86/X86ISelLowering.cpp
index 0d9cf723027e0..8311a5a12032a 100644
--- a/llvm/lib/Target/X86/X86ISelLowering.cpp
+++ b/llvm/lib/Target/X86/X86ISelLowering.cpp
@@ -57565,15 +57565,16 @@ X86TargetLowering::getRegForInlineAsmConstraint(const TargetRegisterInfo *TRI,
if (isGRClass(*Class)) {
unsigned Size = VT.getSizeInBits();
if (Size == 1) Size = 8;
- Register DestReg = getX86SubSuperRegisterOrZero(Res.first, Size);
- if (DestReg > 0) {
+ if (Size != 8 && Size != 16 && Size != 32 && Size != 64)
+ return std::make_pair(0, nullptr);
+ Register DestReg = getX86SubSuperRegister(Res.first, Size);
+ if (DestReg.isValid()) {
bool is64Bit = Subtarget.is64Bit();
const TargetRegisterClass *RC =
Size == 8 ? (is64Bit ? &X86::GR8RegClass : &X86::GR8_NOREXRegClass)
: Size == 16 ? (is64Bit ? &X86::GR16RegClass : &X86::GR16_NOREXRegClass)
: Size == 32 ? (is64Bit ? &X86::GR32RegClass : &X86::GR32_NOREXRegClass)
- : Size == 64 ? (is64Bit ? &X86::GR64RegClass : nullptr)
- : nullptr;
+ : /*Size == 64*/ (is64Bit ? &X86::GR64RegClass : nullptr);
if (Size == 64 && !is64Bit) {
// Model GCC's behavior here and select a fixed pair of 32-bit
// registers.
diff --git a/llvm/lib/Target/X86/X86InstrInfo.cpp b/llvm/lib/Target/X86/X86InstrInfo.cpp
index 00c29f30e96d7..9d2a51fe07d51 100644
--- a/llvm/lib/Target/X86/X86InstrInfo.cpp
+++ b/llvm/lib/Target/X86/X86InstrInfo.cpp
@@ -1098,6 +1098,7 @@ bool X86InstrInfo::classifyLEAReg(MachineInstr &MI, const MachineOperand &Src,
ImplicitOp.setImplicit();
NewSrc = getX86SubSuperRegister(SrcReg, 64);
+ assert(NewSrc.isValid() && "Invalid Operand");
assert(!Src.isUndef() && "Undef op doesn't need optimization");
} else {
// Virtual register of the wrong class, we have to create a temporary 64-bit
diff --git a/llvm/lib/Target/X86/X86MCInstLower.cpp b/llvm/lib/Target/X86/X86MCInstLower.cpp
index 6f89b2e79c45c..97ec866455964 100644
--- a/llvm/lib/Target/X86/X86MCInstLower.cpp
+++ b/llvm/lib/Target/X86/X86MCInstLower.cpp
@@ -1608,6 +1608,7 @@ void X86AsmPrinter::LowerPATCHABLE_EVENT_CALL(const MachineInstr &MI,
if (auto Op = MCIL.LowerMachineOperand(&MI, MI.getOperand(I))) {
assert(Op->isReg() && "Only support arguments in registers");
SrcRegs[I] = getX86SubSuperRegister(Op->getReg(), 64);
+ assert(SrcRegs[I].isValid() && "Invalid operand");
if (SrcRegs[I] != DestRegs[I]) {
UsedMask[I] = true;
EmitAndCountInstruction(
@@ -1706,6 +1707,7 @@ void X86AsmPrinter::LowerPATCHABLE_TYPED_EVENT_CALL(const MachineInstr &MI,
// TODO: Is register only support adequate?
assert(Op->isReg() && "Only supports arguments in registers");
SrcRegs[I] = getX86SubSuperRegister(Op->getReg(), 64);
+ assert(SrcRegs[I].isValid() && "Invalid operand");
if (SrcRegs[I] != DestRegs[I]) {
UsedMask[I] = true;
EmitAndCountInstruction(
diff --git a/llvm/test/CodeGen/X86/asm-modifier-error.ll b/llvm/test/CodeGen/X86/asm-modifier-error.ll
new file mode 100644
index 0000000000000..d16ef65c7dc04
--- /dev/null
+++ b/llvm/test/CodeGen/X86/asm-modifier-error.ll
@@ -0,0 +1,9 @@
+; RUN: not llc < %s -mtriple=x86_64-unknown-unknown 2>&1 | FileCheck %s
+
+; CHECK: error: invalid operand in inline asm: 'mov %ah, ${0:h}'
+define void @test1() {
+entry:
+ %0 = tail call i8 asm sideeffect "mov %ah, ${0:h}", "=r,~{eax},~{ebx},~{ecx},~{edx},~{dirflag},~{fpsr},~{flags}"()
+ ret void
+}
+
More information about the llvm-commits
mailing list