Skip to content

[X86][APX] Exclusively emit setzucc to avoid false dependency #142092

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

[X86][APX] Exclusively emit setzucc to avoid false dependency #142092

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
30b8975
[X86][APX] Exclusively emit setzucc to avoid false dependency
fzou1 May 29, 2025
46c148b
Emit SetZUCC in fast-isel
fzou1 Jun 1, 2025
85e4617
Refine tests.
fzou1 Jun 3, 2025
92f4fdd
Update per comments.
fzou1 Jun 13, 2025
608f957
Update predicate for setcc/setzucc and tests accordingly
fzou1 Jun 13, 2025
17bd924
Revert the change in memfold-nd2rmw.mir
fzou1 Jun 13, 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
33 changes: 21 additions & 12 deletions llvm/lib/Target/X86/X86FastISel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1503,10 +1503,12 @@ bool X86FastISel::X86SelectCmp(const Instruction *I) {

Register FlagReg1 = createResultReg(&X86::GR8RegClass);
Register FlagReg2 = createResultReg(&X86::GR8RegClass);
BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, MIMD, TII.get(X86::SETCCr),
FlagReg1).addImm(SETFOpc[0]);
BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, MIMD, TII.get(X86::SETCCr),
FlagReg2).addImm(SETFOpc[1]);
BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, MIMD,
TII.get(Subtarget->hasZU() ? X86::SETZUCCr : X86::SETCCr), FlagReg1)
Copy link
Contributor

@phoebewang phoebewang Jun 13, 2025

Choose a reason for hiding this comment

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

#define GET_SETCC (Subtarget->hasZU() ? X86::SETZUCCr : X86::SETCCr)

Copy link
Contributor Author

@fzou1 fzou1 Jun 13, 2025

Choose a reason for hiding this comment

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

Updated.

.addImm(SETFOpc[0]);
BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, MIMD,
TII.get(Subtarget->hasZU() ? X86::SETZUCCr : X86::SETCCr), FlagReg2)
.addImm(SETFOpc[1]);
BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, MIMD, TII.get(SETFOpc[2]),
ResultReg).addReg(FlagReg1).addReg(FlagReg2);
updateValueMap(I, ResultReg);
Expand All @@ -1525,8 +1527,9 @@ bool X86FastISel::X86SelectCmp(const Instruction *I) {
if (!X86FastEmitCompare(LHS, RHS, VT, I->getDebugLoc()))
return false;

BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, MIMD, TII.get(X86::SETCCr),
ResultReg).addImm(CC);
BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, MIMD,
TII.get(Subtarget->hasZU() ? X86::SETZUCCr : X86::SETCCr), ResultReg)
.addImm(CC);
updateValueMap(I, ResultReg);
return true;
}
Expand Down Expand Up @@ -2083,10 +2086,14 @@ bool X86FastISel::X86FastEmitCMoveSelect(MVT RetVT, const Instruction *I) {
if (SETFOpc) {
Register FlagReg1 = createResultReg(&X86::GR8RegClass);
Register FlagReg2 = createResultReg(&X86::GR8RegClass);
BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, MIMD, TII.get(X86::SETCCr),
FlagReg1).addImm(SETFOpc[0]);
BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, MIMD, TII.get(X86::SETCCr),
FlagReg2).addImm(SETFOpc[1]);
BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, MIMD,
TII.get(Subtarget->hasZU() ? X86::SETZUCCr : X86::SETCCr),
FlagReg1)
.addImm(SETFOpc[0]);
BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, MIMD,
TII.get(Subtarget->hasZU() ? X86::SETZUCCr : X86::SETCCr),
FlagReg2)
.addImm(SETFOpc[1]);
auto const &II = TII.get(SETFOpc[2]);
if (II.getNumDefs()) {
Register TmpReg = createResultReg(&X86::GR8RegClass);
Expand Down Expand Up @@ -2989,8 +2996,10 @@ bool X86FastISel::fastLowerIntrinsicCall(const IntrinsicInst *II) {
// Assign to a GPR since the overflow return value is lowered to a SETcc.
Register ResultReg2 = createResultReg(&X86::GR8RegClass);
assert((ResultReg+1) == ResultReg2 && "Nonconsecutive result registers.");
BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, MIMD, TII.get(X86::SETCCr),
ResultReg2).addImm(CondCode);
BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, MIMD,
TII.get(Subtarget->hasZU() ? X86::SETZUCCr : X86::SETCCr),
ResultReg2)
.addImm(CondCode);

updateValueMap(II, ResultReg, 2);
return true;
Expand Down
8 changes: 5 additions & 3 deletions llvm/lib/Target/X86/X86FixupSetCC.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,11 @@ bool X86FixupSetCCPass::runOnMachineFunction(MachineFunction &MF) {
if (MI.definesRegister(X86::EFLAGS, /*TRI=*/nullptr))
FlagsDefMI = &MI;

// Find a setcc that is used by a zext.
// Find a setcc/setzucc (if ZU is enabled) that is used by a zext.
// This doesn't have to be the only use, the transformation is safe
// regardless.
if (MI.getOpcode() != X86::SETCCr)
if (MI.getOpcode() != X86::SETCCr &&
(!ST->hasZU() || MI.getOpcode() != X86::SETZUCCr))
Copy link
Contributor

@phoebewang phoebewang Jun 13, 2025

Choose a reason for hiding this comment

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

hasZU is not needed.

Copy link
Contributor Author

@fzou1 fzou1 Jun 13, 2025

Choose a reason for hiding this comment

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

Updated.

continue;

MachineInstr *ZExt = nullptr;
Expand Down Expand Up @@ -122,7 +123,8 @@ bool X86FixupSetCCPass::runOnMachineFunction(MachineFunction &MF) {
// register.
Register ZeroReg = MRI->createVirtualRegister(RC);
if (ST->hasZU()) {
MI.setDesc(TII->get(X86::SETZUCCr));
if (MI.getOpcode() != X86::SETZUCCr)
MI.setDesc(TII->get(X86::SETZUCCr));
Copy link
Contributor

@phoebewang phoebewang Jun 13, 2025

Choose a reason for hiding this comment

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

Don't we always generate SETZUCCr now?

Copy link
Contributor Author

@fzou1 fzou1 Jun 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Don't we always generate SETZUCCr now?

Yes. We do always emit SETZUCCr when NDD feature is specified. This is to transform SETCCr to SETZUCCr when it came from IR in LIT test, like apx/setzucc.ll.

Copy link
Contributor

@phoebewang phoebewang Jun 13, 2025

Choose a reason for hiding this comment

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

I don't understand why NDD matters here. apx/setzucc.ll doesn't enable NDD either.

Copy link
Contributor Author

@fzou1 fzou1 Jun 13, 2025

Choose a reason for hiding this comment

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

Updated to check ZU flag for SETZUCCr instruction in td and added assertion here to ensure MI is SETZUCCr instruction.

BuildMI(*ZExt->getParent(), ZExt, ZExt->getDebugLoc(),
TII->get(TargetOpcode::IMPLICIT_DEF), ZeroReg);
} else {
Expand Down
29 changes: 4 additions & 25 deletions llvm/lib/Target/X86/X86FlagsCopyLowering.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -746,8 +746,10 @@ Register X86FlagsCopyLoweringPass::promoteCondToReg(
MachineBasicBlock &TestMBB, MachineBasicBlock::iterator TestPos,
const DebugLoc &TestLoc, X86::CondCode Cond) {
Register Reg = MRI->createVirtualRegister(PromoteRC);
auto SetI = BuildMI(TestMBB, TestPos, TestLoc, TII->get(X86::SETCCr), Reg)
.addImm(Cond);
auto SetI =
BuildMI(TestMBB, TestPos, TestLoc,
TII->get(Subtarget->hasZU() ? X86::SETZUCCr : X86::SETCCr), Reg)
.addImm(Cond);
(void)SetI;
LLVM_DEBUG(dbgs() << " save cond: "; SetI->dump());
++NumSetCCsInserted;
Expand Down Expand Up @@ -791,29 +793,6 @@ void X86FlagsCopyLoweringPass::rewriteSetCC(MachineBasicBlock &MBB,
if (!CondReg)
CondReg = promoteCondToReg(MBB, Pos, Loc, Cond);

if (X86::isSETZUCC(MI.getOpcode())) {
// SETZUCC is generated for register only for now.
assert(!MI.mayStore() && "Cannot handle memory variants");
assert(MI.getOperand(0).isReg() &&
"Cannot have a non-register defined operand to SETZUcc!");
Register OldReg = MI.getOperand(0).getReg();
// Drop Kill flags on the old register before replacing. CondReg may have
// a longer live range.
MRI->clearKillFlags(OldReg);
for (auto &Use : MRI->use_instructions(OldReg)) {
assert(Use.getOpcode() == X86::INSERT_SUBREG &&
"SETZUCC should be only used by INSERT_SUBREG");
Use.getOperand(2).setReg(CondReg);
// Recover MOV32r0 before INSERT_SUBREG, which removed by SETZUCC.
Register ZeroReg = MRI->createVirtualRegister(&X86::GR32RegClass);
BuildMI(*Use.getParent(), &Use, Use.getDebugLoc(), TII->get(X86::MOV32r0),
ZeroReg);
Use.getOperand(1).setReg(ZeroReg);
}
MI.eraseFromParent();
return;
}

// Rewriting a register def is trivial: we just replace the register and
// remove the setcc.
if (!MI.mayStore()) {
Expand Down
8 changes: 6 additions & 2 deletions llvm/lib/Target/X86/X86InstrCMovSetCC.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,11 +137,14 @@ let Predicates = [HasCMOV, HasCF] in {
}

// SetCC instructions.
let Uses = [EFLAGS], isCodeGenOnly = 1, ForceDisassemble = 1 in {
let Uses = [EFLAGS], isCodeGenOnly = 1, ForceDisassemble = 1, Predicates = [NoNDD] in {
Copy link
Contributor

@phoebewang phoebewang Jun 13, 2025

Choose a reason for hiding this comment

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

NoNDD?

Copy link
Contributor Author

@fzou1 fzou1 Jun 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

NoNDD?

Yes. See the definition in X86InstrPredicates.td as below:

def HasNDD       : Predicate<"Subtarget->hasNDD()">;
def NoNDD        : Predicate<"!Subtarget->hasNDD()">;

Copy link
Contributor

@phoebewang phoebewang Jun 13, 2025

Choose a reason for hiding this comment

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

I mean why not checking HasZU?

Copy link
Contributor Author

@fzou1 fzou1 Jun 13, 2025

Choose a reason for hiding this comment

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

Updated. Thanks.

def SETCCr : I<0x90, MRMXrCC, (outs GR8:$dst), (ins ccode:$cond),
"set${cond}\t$dst",
[(set GR8:$dst, (X86setcc timm:$cond, EFLAGS))]>,
TB, Sched<[WriteSETCC]>;
}

let Uses = [EFLAGS], isCodeGenOnly = 1, ForceDisassemble = 1 in {
def SETCCm : I<0x90, MRMXmCC, (outs), (ins i8mem:$dst, ccode:$cond),
"set${cond}\t$dst",
[(store (X86setcc timm:$cond, EFLAGS), addr:$dst)]>,
Expand All @@ -152,7 +155,8 @@ let Uses = [EFLAGS], isCodeGenOnly = 1, ForceDisassemble = 1 in {
let Uses = [EFLAGS], isCodeGenOnly = 1, ForceDisassemble = 1,
hasSideEffects = 0, Predicates = [In64BitMode], Predicates = [HasNDD] in {
def SETZUCCr : I<0x40, MRMXrCC, (outs GR8:$dst), (ins ccode:$cond),
"setzu${cond}\t$dst", []>,
"setzu${cond}\t$dst",
[(set GR8:$dst, (X86setcc timm:$cond, EFLAGS))]>,
XD, ZU, NoCD8, Sched<[WriteSETCC]>;
def SETCCr_EVEX : I<0x40, MRMXrCC, (outs GR8:$dst), (ins ccode:$cond),
"set${cond}\t$dst", []>,
Expand Down
13 changes: 13 additions & 0 deletions llvm/test/CodeGen/X86/2009-04-12-FastIselOverflowCrash.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
; RUN: llc < %s -fast-isel | FileCheck %s --check-prefix=FASTISEL
; PR30981
; RUN: llc < %s -O0 -mcpu=x86-64 -mattr=+avx512f | FileCheck %s --check-prefix=AVX512F
; RUN: llc < %s -O0 -mcpu=x86-64 -mattr=+zu | FileCheck %s --check-prefix=ZU

target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128"
target triple = "x86_64-apple-darwin10"

Expand All @@ -26,6 +28,17 @@ define fastcc i32 @test() nounwind {
; AVX512F-NEXT: LBB0_2: ## %.backedge
; AVX512F-NEXT: xorl %eax, %eax
; AVX512F-NEXT: retq
;
; ZU-LABEL: test:
; ZU: ## %bb.0: ## %entry
; ZU-NEXT: movl $1, %eax
; ZU-NEXT: addl $0, %eax
; ZU-NEXT: setzuo %al
; ZU-NEXT: jo LBB0_2
; ZU-NEXT: ## %bb.1: ## %BB3
; ZU-NEXT: LBB0_2: ## %.backedge
; ZU-NEXT: xorl %eax, %eax
; ZU-NEXT: retq
entry:
%tmp1 = call { i32, i1 } @llvm.sadd.with.overflow.i32(i32 1, i32 0)
%tmp2 = extractvalue { i32, i1 } %tmp1, 1
Expand Down
4 changes: 2 additions & 2 deletions llvm/test/CodeGen/X86/apx/add.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -758,7 +758,7 @@ define i1 @add64ri_reloc(i16 %k) {
; CHECK-NEXT: addq %rax, %rax # EVEX TO LEGACY Compression encoding: [0x48,0x01,0xc0]
; CHECK-NEXT: addq $val, %rax # EVEX TO LEGACY Compression encoding: [0x48,0x05,A,A,A,A]
; CHECK-NEXT: # fixup A - offset: 2, value: val, kind: reloc_signed_4byte
; CHECK-NEXT: setne %al # encoding: [0x0f,0x95,0xc0]
; CHECK-NEXT: setzune %al # encoding: [0x62,0xf4,0x7f,0x18,0x45,0xc0]
; CHECK-NEXT: retq # encoding: [0xc3]
;
; NF-LABEL: add64ri_reloc:
Expand All @@ -768,7 +768,7 @@ define i1 @add64ri_reloc(i16 %k) {
; NF-NEXT: addq %rax, %rax # EVEX TO LEGACY Compression encoding: [0x48,0x01,0xc0]
; NF-NEXT: addq $val, %rax # EVEX TO LEGACY Compression encoding: [0x48,0x05,A,A,A,A]
; NF-NEXT: # fixup A - offset: 2, value: val, kind: reloc_signed_4byte
; NF-NEXT: setne %al # encoding: [0x0f,0x95,0xc0]
; NF-NEXT: setzune %al # encoding: [0x62,0xf4,0x7f,0x18,0x45,0xc0]
; NF-NEXT: retq # encoding: [0xc3]
%g = getelementptr inbounds i16, ptr @val, i16 %k
%cmp = icmp ne ptr %g, null
Expand Down
Loading
Loading