Skip to content

[X86][AsmParser] Improve rel8 validation #126073

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Mar 6, 2025
Merged

[X86][AsmParser] Improve rel8 validation #126073

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
24d86a4
[X86][AsmParser] Improve rel8 validation
e-kud Feb 6, 2025
78af857
Merge remote-tracking branch 'origin/main' into rel8
e-kud Feb 26, 2025
d60fbb3
Allow label+offset combination for relocation
e-kud Feb 26, 2025
886f28a
Formatting
e-kud Feb 26, 2025
c925803
Missing check lines
e-kud Feb 26, 2025
d3bc8a7
Add positive tests for j*cxz
e-kud Mar 3, 2025
76e171f
Merge remote-tracking branch 'origin/main' into rel8
e-kud Mar 3, 2025
c215b2c
Check imm byte ptr
e-kud Mar 5, 2025
11f4124
Merge branch 'main' into rel8
e-kud Mar 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions llvm/lib/Target/X86/AsmParser/X86Operand.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,10 +416,16 @@ struct X86Operand final : public MCParsedAsmOperand {
return isImm();
}

bool isAbsMem16() const {
return isAbsMem() && Mem.ModeSize == 16;
bool isAbsMemMode16() const { return isAbsMem() && Mem.ModeSize == 16; }

bool isDispImm8() const {
if (auto *CE = dyn_cast<MCConstantExpr>(getMemDisp()))
return isImmSExti64i8Value(CE->getValue());
return true;
}

bool isAbsMem8() const { return isAbsMem() && isMem8() && isDispImm8(); }

bool isMemUseUpRegs() const override { return UseUpRegs; }

bool isSrcIdx() const {
Expand Down
15 changes: 9 additions & 6 deletions llvm/lib/Target/X86/X86InstrControl.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,14 +94,14 @@ let isBranch = 1, isTerminator = 1, hasSideEffects = 0, SchedRW = [WriteJump] in
// 32-bit mode, the address size prefix is jcxz and the unprefixed version is
// jecxz.
let Uses = [CX] in
def JCXZ : Ii8PCRel<0xE3, RawFrm, (outs), (ins brtarget8:$dst),
def JCXZ : Ii8PCRel<0xE3, RawFrm, (outs), (ins i8imm_brtarget:$dst),
"jcxz\t$dst", []>, AdSize16, Requires<[Not64BitMode]>;
let Uses = [ECX] in
def JECXZ : Ii8PCRel<0xE3, RawFrm, (outs), (ins brtarget8:$dst),
def JECXZ : Ii8PCRel<0xE3, RawFrm, (outs), (ins i8imm_brtarget:$dst),
"jecxz\t$dst", []>, AdSize32;

let Uses = [RCX] in
def JRCXZ : Ii8PCRel<0xE3, RawFrm, (outs), (ins brtarget8:$dst),
def JRCXZ : Ii8PCRel<0xE3, RawFrm, (outs), (ins i8imm_brtarget:$dst),
"jrcxz\t$dst", []>, AdSize64, Requires<[In64BitMode]>;
}

Expand Down Expand Up @@ -193,9 +193,12 @@ def JMPABS64i : Ii64<0xA1, RawFrm, (outs), (ins i64imm:$dst), "jmpabs\t$dst", []

// Loop instructions
let isBranch = 1, isTerminator = 1, SchedRW = [WriteJump] in {
def LOOP : Ii8PCRel<0xE2, RawFrm, (outs), (ins brtarget8:$dst), "loop\t$dst", []>;
def LOOPE : Ii8PCRel<0xE1, RawFrm, (outs), (ins brtarget8:$dst), "loope\t$dst", []>;
def LOOPNE : Ii8PCRel<0xE0, RawFrm, (outs), (ins brtarget8:$dst), "loopne\t$dst", []>;
def LOOP : Ii8PCRel<0xE2, RawFrm, (outs), (ins i8imm_brtarget:$dst),
"loop\t$dst", []>;
def LOOPE : Ii8PCRel<0xE1, RawFrm, (outs), (ins i8imm_brtarget:$dst),
"loope\t$dst", []>;
def LOOPNE : Ii8PCRel<0xE0, RawFrm, (outs), (ins i8imm_brtarget:$dst),
"loopne\t$dst", []>;
Copy link
Collaborator

@RKSimon RKSimon Feb 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not entirely related, but the LOOP defs needs some cleanup #60208

Copy link
Contributor Author

@e-kud e-kud Mar 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. But yes, I think it's not quite related. Also #61722 and #60208 should be solved in one move. We need to distinguish loop-instructions in 16-32-64 bit modes to correctly specify whether it uses CX, ECX or RCX. For which, I assume, we need to repeat REP approach: create LOOP_16, LOOP_32 and LOOP_64 to specify the mode, defs/uses registers and predicate it whether it is a 64 bit platform.

}

//===----------------------------------------------------------------------===//
Expand Down
15 changes: 12 additions & 3 deletions llvm/lib/Target/X86/X86InstrOperands.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,8 +142,14 @@ def i64mem_TC : X86MemOperand<"printqwordmem", X86Mem64AsmOperand, 64> {
}

// Special parser to detect 16-bit mode to select 16-bit displacement.
def X86AbsMem16AsmOperand : AsmOperandClass {
let Name = "AbsMem16";
def X86AbsMemMode16AsmOperand : AsmOperandClass {
let Name = "AbsMemMode16";
let RenderMethod = "addAbsMemOperands";
let SuperClasses = [X86AbsMemAsmOperand];
}

def X86AbsMem8AsmOperand : AsmOperandClass {
let Name = "AbsMem8";
let RenderMethod = "addAbsMemOperands";
let SuperClasses = [X86AbsMemAsmOperand];
}
Expand All @@ -157,6 +163,9 @@ class BranchTargetOperand<ValueType ty> : Operand<ty> {

def i32imm_brtarget : BranchTargetOperand<i32>;
def i16imm_brtarget : BranchTargetOperand<i16>;
def i8imm_brtarget : BranchTargetOperand<i8> {
let ParserMatchClass = X86AbsMem8AsmOperand;
}

// 64-bits but only 32 bits are significant, and those bits are treated as being
// pc relative.
Expand All @@ -165,7 +174,7 @@ def i64i32imm_brtarget : BranchTargetOperand<i64>;
def brtarget : BranchTargetOperand<OtherVT>;
def brtarget8 : BranchTargetOperand<OtherVT>;
def brtarget16 : BranchTargetOperand<OtherVT> {
let ParserMatchClass = X86AbsMem16AsmOperand;
let ParserMatchClass = X86AbsMemMode16AsmOperand;
}
def brtarget32 : BranchTargetOperand<OtherVT>;

Expand Down
4 changes: 4 additions & 0 deletions llvm/test/MC/X86/I386-32.s
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,10 @@ iretl
// CHECK: encoding: [0x66,0xcf]
iretw

// CHECK: jcxz 64
// CHECK: encoding: [0x67,0xe3,A]
jcxz 64

// CHECK: jecxz 64
// CHECK: encoding: [0xe3,A]
jecxz 64
Expand Down
8 changes: 8 additions & 0 deletions llvm/test/MC/X86/I386-64.s
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,14 @@ iretq
// CHECK: encoding: [0x66,0xcf]
iretw

// CHECK: jecxz 64
// CHECK: encoding: [0x67,0xe3,A]
jecxz 64

// CHECK: jrcxz 64
// CHECK: encoding: [0xe3,A]
jrcxz 64

// CHECK: lodsl %gs:(%rsi), %eax
// CHECK: encoding: [0x65,0xad]
lodsl %gs:(%rsi), %eax
Expand Down
8 changes: 8 additions & 0 deletions llvm/test/MC/X86/intel-syntax.s
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@
// RUN: FileCheck < %t %s
// RUN: FileCheck --check-prefix=CHECK-STDERR < %t.err %s

_nop_label:
nop

_test:
xor EAX, EAX
ret
Expand Down Expand Up @@ -825,6 +828,11 @@ fucomip st, st(2)
// CHECK: fcompi %st(2)
// CHECK: fucompi %st(2)

jrcxz _nop_label
jecxz _nop_label + 1
// CHECK: jrcxz _nop_label
// CHECK: jecxz _nop_label+1

Copy link
Contributor

@phoebewang phoebewang Feb 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add positive test for Imm8?

Copy link
Contributor Author

@e-kud e-kud Mar 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added positive tests for j*cxz around existing j*cxz tests in I386-32.s and I386-64.s as they were missing.

Copy link
Contributor

@phoebewang phoebewang Mar 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we support the jrcxz BYTE PTR [64] format?

Copy link
Contributor Author

@e-kud e-kud Mar 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, added the test case but with loop.

loopz _foo
loopnz _foo
// CHECK: loope _foo
Expand Down
36 changes: 36 additions & 0 deletions llvm/test/MC/X86/validate-inst-intel.s
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,39 @@
# CHECK: int -129
# CHECK: ^

.text
loop WORD PTR [SYM+4]
# CHECK: error: invalid operand for instruction
# CHECK: loop WORD PTR [SYM+4]
# CHECK: ^

.text
loope BYTE PTR [128]
# CHECK: error: invalid operand for instruction
# CHECK: loope BYTE PTR [128]
# CHECK: ^

.text
loopne BYTE PTR [-129]
# CHECK: error: invalid operand for instruction
# CHECK: loopne BYTE PTR [-129]
# CHECK: ^

.text
jrcxz XMMWORD PTR [0]
# CHECK: error: invalid operand for instruction
# CHECK: jrcxz XMMWORD PTR [0]
# CHECK: ^

.text
jecxz BYTE PTR[-444]
# CHECK: error: invalid operand for instruction
# CHECK: jecxz BYTE PTR[-444]
# CHECK: ^

.text
jcxz BYTE PTR[444]
# CHECK: error: invalid operand for instruction
# CHECK: jcxz BYTE PTR[444]
# CHECK: ^

2 changes: 2 additions & 0 deletions llvm/utils/TableGen/X86RecognizableInstr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1087,6 +1087,7 @@ OperandType RecognizableInstr::typeFromString(const std::string &s,
TYPE("i512mem_GR32", TYPE_M)
TYPE("i512mem_GR64", TYPE_M)
TYPE("i64i32imm_brtarget", TYPE_REL)
TYPE("i8imm_brtarget", TYPE_REL)
TYPE("i16imm_brtarget", TYPE_REL)
TYPE("i32imm_brtarget", TYPE_REL)
TYPE("ccode", TYPE_IMM)
Expand Down Expand Up @@ -1405,6 +1406,7 @@ RecognizableInstr::relocationEncodingFromString(const std::string &s,
ENCODING("i64i32imm_brtarget", ENCODING_ID)
ENCODING("i16imm_brtarget", ENCODING_IW)
ENCODING("i32imm_brtarget", ENCODING_ID)
ENCODING("i8imm_brtarget", ENCODING_IB)
ENCODING("brtarget32", ENCODING_ID)
ENCODING("brtarget16", ENCODING_IW)
ENCODING("brtarget8", ENCODING_IB)
Expand Down