Skip to content

[X86][APX] Try to replace non-NF with NF instructions when optimizeCompareInstr #130488

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 2 commits into from
Mar 10, 2025
Merged

[X86][APX] Try to replace non-NF with NF instructions when optimizeCompareInstr #130488

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3f0314d
[X86][APX] Try to replace NDD with NF instructions when optimizeCompa…
phoebewang Mar 9, 2025
1c4bc9b
Address review comments
phoebewang Mar 10, 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
18 changes: 18 additions & 0 deletions llvm/lib/Target/X86/X86InstrInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5352,10 +5352,12 @@ bool X86InstrInfo::optimizeCompareInstr(MachineInstr &CmpInstr, Register SrcReg,
MachineInstr *MI = nullptr;
MachineInstr *Sub = nullptr;
MachineInstr *Movr0Inst = nullptr;
SmallVector<std::pair<MachineInstr *, unsigned>, 4> InstsToUpdate;
bool NoSignFlag = false;
bool ClearsOverflowFlag = false;
bool ShouldUpdateCC = false;
bool IsSwapped = false;
bool HasNF = Subtarget.hasNF();
unsigned OpNo = 0;
X86::CondCode NewCC = X86::COND_INVALID;
int64_t ImmDelta = 0;
Expand Down Expand Up @@ -5441,6 +5443,16 @@ bool X86InstrInfo::optimizeCompareInstr(MachineInstr &CmpInstr, Register SrcReg,
continue;
}

// Try to replace non-NF with NF instructions.
if (HasNF && Inst.registerDefIsDead(X86::EFLAGS, TRI)) {
unsigned NewOp = X86::getNFVariant(Inst.getOpcode());
Copy link
Contributor

@KanRobert KanRobert Mar 10, 2025

Choose a reason for hiding this comment

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

It seems we don't need to store the opcodes of NF variants. Just setDesc(X86::getNFVariant(Inst.getOpcode())); at line 5654?

Copy link
Contributor Author

@phoebewang phoebewang Mar 10, 2025

Choose a reason for hiding this comment

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

I think the table lookup is more expensive than a little memory space.

Copy link
Contributor

@KanRobert KanRobert Mar 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Would it introduce more times of table lookup? The times seem equal for me.

Copy link
Contributor Author

@phoebewang phoebewang Mar 10, 2025

Choose a reason for hiding this comment

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

Yes, we need to query twice if we don't store the value. We need query here to check if the instruction can turn into NF.

Copy link
Contributor

@KanRobert KanRobert Mar 10, 2025

Choose a reason for hiding this comment

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

Ah, okay

if (!NewOp)
return false;

InstsToUpdate.push_back(std::make_pair(&Inst, NewOp));
continue;
}

// Cannot do anything for any other EFLAG changes.
return false;
}
Expand Down Expand Up @@ -5637,6 +5649,12 @@ bool X86InstrInfo::optimizeCompareInstr(MachineInstr &CmpInstr, Register SrcReg,
return false;
}

// Replace non-NF with NF instructions.
for (auto &Inst : InstsToUpdate) {
Inst.first->setDesc(get(Inst.second));
Inst.first->removeOperand(Inst.first->getNumOperands() - 1);
}

// Make sure Sub instruction defines EFLAGS and mark the def live.
MachineOperand *FlagDef =
Sub->findRegisterDefOperand(X86::EFLAGS, /*TRI=*/nullptr);
Expand Down
23 changes: 21 additions & 2 deletions llvm/test/CodeGen/X86/apx/cf.ll
View file
Open in desktop
Copy link
Contributor

@KanRobert KanRobert Mar 10, 2025

Choose a reason for hiding this comment

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

BTW, this test should be added in nf.ll instead of cf.ll

Copy link
Contributor Author

@phoebewang phoebewang Mar 10, 2025

Choose a reason for hiding this comment

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

There's no nf.ll. The nf tests are sacttered in add/or/sub/...ll, so put it in cf.ll is ok since we happen to have nf condition lowering here :)

Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
; RUN: llc < %s -mtriple=x86_64 -mattr=+cf,+avx512f -verify-machineinstrs | FileCheck %s
; RUN: llc < %s -mtriple=x86_64 -mattr=+cf,+nf,+avx512f -verify-machineinstrs | FileCheck %s

define void @basic(i32 %a, ptr %b, ptr %p, ptr %q) {
; CHECK-LABEL: basic:
Expand Down Expand Up @@ -125,7 +125,7 @@ entry:
ret void
}

define void @single_cmp(i32 %a, i32 %b, ptr %c, ptr %d) #2 {
define void @single_cmp(i32 %a, i32 %b, ptr %c, ptr %d) {
; CHECK-LABEL: single_cmp:
; CHECK: # %bb.0: # %entry
; CHECK-NEXT: cmpl %esi, %edi
Expand All @@ -139,3 +139,22 @@ entry:
tail call void @llvm.masked.store.v1i16.p0(<1 x i16> %2, ptr %d, i32 2, <1 x i1> %1)
ret void
}

define void @load_add_store(i32 %a, i32 %b, ptr %p) {
; CHECK-LABEL: load_add_store:
; CHECK: # %bb.0: # %entry
; CHECK-NEXT: cmpl %esi, %edi
; CHECK-NEXT: cfcmovnew (%rdx), %ax
; CHECK-NEXT: {nf} incl %eax
; CHECK-NEXT: cfcmovnew %ax, (%rdx)
; CHECK-NEXT: retq
entry:
%0 = icmp ne i32 %a, %b
%1 = insertelement <1 x i1> poison, i1 %0, i64 0
%2 = tail call <1 x i16> @llvm.masked.load.v1i16.p0(ptr %p, i32 2, <1 x i1> %1, <1 x i16> poison)
%3 = extractelement <1 x i16> %2, i64 0
%4 = add i16 %3, 1
%5 = insertelement <1 x i16> poison, i16 %4, i64 0
tail call void @llvm.masked.store.v1i16.p0(<1 x i16> %5, ptr %p, i32 2, <1 x i1> %1)
ret void
}