https://github.com/bwendling updated
https://github.com/llvm/llvm-project/pull/67193
>From 6db37f7f76347a7821d9a95c0fdac4e250df2e78 Mon Sep 17 00:00:00 2001
From: Bill Wendling <mo...@google.com>
Date: Fri, 22 Sep 2023 12:35:09 -0700
Subject: [PATCH 1/5] [CodeGen] Avoid potential sideeffects from XOR
XOR may change flag values (e.g. for X86 gprs). In the case where that's
not desirable, specify that buildClearRegister() should use MOV instead.
---
llvm/include/llvm/CodeGen/TargetInstrInfo.h | 7 +++--
llvm/lib/Target/AArch64/AArch64InstrInfo.cpp | 14 +++++----
llvm/lib/Target/AArch64/AArch64InstrInfo.h | 4 +--
llvm/lib/Target/X86/X86InstrInfo.cpp | 31 +++++++++++++-------
llvm/lib/Target/X86/X86InstrInfo.h | 4 +--
5 files changed, 38 insertions(+), 22 deletions(-)
diff --git a/llvm/include/llvm/CodeGen/TargetInstrInfo.h
b/llvm/include/llvm/CodeGen/TargetInstrInfo.h
index 04859a50d6fdeb4..7c2bd83d1d623ef 100644
--- a/llvm/include/llvm/CodeGen/TargetInstrInfo.h
+++ b/llvm/include/llvm/CodeGen/TargetInstrInfo.h
@@ -2057,10 +2057,13 @@ class TargetInstrInfo : public MCInstrInfo {
"Target didn't implement TargetInstrInfo::insertOutlinedCall!");
}
- /// Insert an architecture-specific instruction to clear a register.
+ /// Insert an architecture-specific instruction to clear a register. If you
+ /// need to avoid sideeffects (e.g. XOR on x86), set \p NoSideEffects to \p
+ /// true.
virtual void buildClearRegister(Register Reg, MachineBasicBlock &MBB,
MachineBasicBlock::iterator Iter,
- DebugLoc &DL) const {
+ DebugLoc &DL,
+ bool NoSideEffects = false) const {
llvm_unreachable(
"Target didn't implement TargetInstrInfo::buildClearRegister!");
}
diff --git a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
index a1e6a8177c10013..3b1e301e576a1b1 100644
--- a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
+++ b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
@@ -8337,21 +8337,23 @@ bool
AArch64InstrInfo::shouldOutlineFromFunctionByDefault(
void AArch64InstrInfo::buildClearRegister(Register Reg, MachineBasicBlock &MBB,
MachineBasicBlock::iterator Iter,
- DebugLoc &DL) const {
+ DebugLoc &DL,
+ bool NoSideEffects) const {
const MachineFunction &MF = *MBB.getParent();
const AArch64Subtarget &STI = MF.getSubtarget<AArch64Subtarget>();
const AArch64RegisterInfo &TRI = *STI.getRegisterInfo();
if (TRI.isGeneralPurposeRegister(MF, Reg)) {
- BuildMI(MBB, Iter, DL, get(AArch64::MOVi64imm), Reg)
- .addImm(0);
+ BuildMI(MBB, Iter, DL, get(AArch64::MOVZXi), Reg)
+ .addImm(0)
+ .addImm(0);
} else if (STI.hasSVE()) {
BuildMI(MBB, Iter, DL, get(AArch64::DUP_ZI_D), Reg)
- .addImm(0)
- .addImm(0);
+ .addImm(0)
+ .addImm(0);
} else {
BuildMI(MBB, Iter, DL, get(AArch64::MOVIv2d_ns), Reg)
- .addImm(0);
+ .addImm(0);
}
}
diff --git a/llvm/lib/Target/AArch64/AArch64InstrInfo.h
b/llvm/lib/Target/AArch64/AArch64InstrInfo.h
index 4a4d87c1b1f6ba5..5c5e7e4fe39068a 100644
--- a/llvm/lib/Target/AArch64/AArch64InstrInfo.h
+++ b/llvm/lib/Target/AArch64/AArch64InstrInfo.h
@@ -320,8 +320,8 @@ class AArch64InstrInfo final : public AArch64GenInstrInfo {
bool shouldOutlineFromFunctionByDefault(MachineFunction &MF) const override;
void buildClearRegister(Register Reg, MachineBasicBlock &MBB,
- MachineBasicBlock::iterator Iter,
- DebugLoc &DL) const override;
+ MachineBasicBlock::iterator Iter, DebugLoc &DL,
+ bool NoSideEffects = false) const override;
/// Returns the vector element size (B, H, S or D) of an SVE opcode.
uint64_t getElementSizeForOpcode(unsigned Opc) const;
diff --git a/llvm/lib/Target/X86/X86InstrInfo.cpp
b/llvm/lib/Target/X86/X86InstrInfo.cpp
index 73675a868239ea1..24a7d632f951385 100644
--- a/llvm/lib/Target/X86/X86InstrInfo.cpp
+++ b/llvm/lib/Target/X86/X86InstrInfo.cpp
@@ -9796,27 +9796,34 @@ X86InstrInfo::insertOutlinedCall(Module &M,
MachineBasicBlock &MBB,
return It;
}
-void X86InstrInfo::buildClearRegister(Register Reg,
- MachineBasicBlock &MBB,
+void X86InstrInfo::buildClearRegister(Register Reg, MachineBasicBlock &MBB,
MachineBasicBlock::iterator Iter,
- DebugLoc &DL) const {
+ DebugLoc &DL, bool NoSideEffects) const {
const MachineFunction &MF = *MBB.getParent();
const X86Subtarget &ST = MF.getSubtarget<X86Subtarget>();
const TargetRegisterInfo &TRI = getRegisterInfo();
if (ST.hasMMX() && X86::VR64RegClass.contains(Reg))
- // FIXME: Ignore MMX registers?
+ // FIXME: Should we ignore MMX registers?
return;
if (TRI.isGeneralPurposeRegister(MF, Reg)) {
- BuildMI(MBB, Iter, DL, get(X86::XOR32rr), Reg)
- .addReg(Reg, RegState::Undef)
- .addReg(Reg, RegState::Undef);
+ // Convert register to the 32-bit version.
+ Reg = getX86SubSuperRegister(Reg, 32);
+
+ if (NoSideEffects)
+ // XOR affects flags, so use a MOV instead.
+ BuildMI(MBB, Iter, DL, get(X86::MOV32ri), Reg).addImm(0);
+ else
+ BuildMI(MBB, Iter, DL, get(X86::XOR32rr), Reg)
+ .addReg(Reg, RegState::Undef)
+ .addReg(Reg, RegState::Undef);
} else if (X86::VR128RegClass.contains(Reg)) {
// XMM#
if (!ST.hasSSE1())
return;
+ // PXOR is safe to use because it doesn't affect flags.
BuildMI(MBB, Iter, DL, get(X86::PXORrr), Reg)
.addReg(Reg, RegState::Undef)
.addReg(Reg, RegState::Undef);
@@ -9825,6 +9832,7 @@ void X86InstrInfo::buildClearRegister(Register Reg,
if (!ST.hasAVX())
return;
+ // VPXOR is safe to use because it doesn't affect flags.
BuildMI(MBB, Iter, DL, get(X86::VPXORrr), Reg)
.addReg(Reg, RegState::Undef)
.addReg(Reg, RegState::Undef);
@@ -9833,6 +9841,7 @@ void X86InstrInfo::buildClearRegister(Register Reg,
if (!ST.hasAVX512())
return;
+ // VPXORY is safe to use because it doesn't affect flags.
BuildMI(MBB, Iter, DL, get(X86::VPXORYrr), Reg)
.addReg(Reg, RegState::Undef)
.addReg(Reg, RegState::Undef);
@@ -9844,9 +9853,11 @@ void X86InstrInfo::buildClearRegister(Register Reg,
if (!ST.hasVLX())
return;
- BuildMI(MBB, Iter, DL, get(ST.hasBWI() ? X86::KXORQrr : X86::KXORWrr), Reg)
- .addReg(Reg, RegState::Undef)
- .addReg(Reg, RegState::Undef);
+ // KXOR is safe to use because it doesn't affect flags.
+ unsigned Op = ST.hasBWI() ? X86::KXORQrr : X86::KXORWrr;
+ BuildMI(MBB, Iter, DL, get(Op), Reg)
+ .addReg(Reg, RegState::Undef)
+ .addReg(Reg, RegState::Undef);
}
}
diff --git a/llvm/lib/Target/X86/X86InstrInfo.h
b/llvm/lib/Target/X86/X86InstrInfo.h
index 8119302f73e8b36..a5ad618ef4b6415 100644
--- a/llvm/lib/Target/X86/X86InstrInfo.h
+++ b/llvm/lib/Target/X86/X86InstrInfo.h
@@ -574,8 +574,8 @@ class X86InstrInfo final : public X86GenInstrInfo {
outliner::Candidate &C) const override;
void buildClearRegister(Register Reg, MachineBasicBlock &MBB,
- MachineBasicBlock::iterator Iter,
- DebugLoc &DL) const override;
+ MachineBasicBlock::iterator Iter, DebugLoc &DL,
+ bool NoSideEffects = false) const override;
bool verifyInstruction(const MachineInstr &MI,
StringRef &ErrInfo) const override;
>From 3fd234487dd5796043a4f5df4cf662f2c0373bf0 Mon Sep 17 00:00:00 2001
From: Bill Wendling <mo...@google.com>
Date: Fri, 22 Sep 2023 14:12:43 -0700
Subject: [PATCH 2/5] Fix formatting.
---
llvm/lib/Target/AArch64/AArch64InstrInfo.cpp | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
index 3b1e301e576a1b1..ddd496d127732b5 100644
--- a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
+++ b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
@@ -8344,16 +8344,11 @@ void AArch64InstrInfo::buildClearRegister(Register Reg,
MachineBasicBlock &MBB,
const AArch64RegisterInfo &TRI = *STI.getRegisterInfo();
if (TRI.isGeneralPurposeRegister(MF, Reg)) {
- BuildMI(MBB, Iter, DL, get(AArch64::MOVZXi), Reg)
- .addImm(0)
- .addImm(0);
+ BuildMI(MBB, Iter, DL, get(AArch64::MOVZXi), Reg).addImm(0).addImm(0);
} else if (STI.hasSVE()) {
- BuildMI(MBB, Iter, DL, get(AArch64::DUP_ZI_D), Reg)
- .addImm(0)
- .addImm(0);
+ BuildMI(MBB, Iter, DL, get(AArch64::DUP_ZI_D), Reg).addImm(0).addImm(0);
} else {
- BuildMI(MBB, Iter, DL, get(AArch64::MOVIv2d_ns), Reg)
- .addImm(0);
+ BuildMI(MBB, Iter, DL, get(AArch64::MOVIv2d_ns), Reg).addImm(0);
}
}
>From 39a76d2db07f4c6eda9a0227da28d575553f7f6f Mon Sep 17 00:00:00 2001
From: Bill Wendling <mo...@google.com>
Date: Tue, 26 Sep 2023 11:49:37 -0700
Subject: [PATCH 3/5] Reformatting and explanitory note
---
llvm/lib/Target/AArch64/AArch64InstrInfo.cpp | 11 ++++++++---
llvm/lib/Target/X86/X86InstrInfo.cpp | 3 ++-
2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
index a5c3c27a1c0e215..1faf0d73f5aab66 100644
--- a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
+++ b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
@@ -9073,11 +9073,16 @@ void AArch64InstrInfo::buildClearRegister(Register Reg,
MachineBasicBlock &MBB,
const AArch64RegisterInfo &TRI = *STI.getRegisterInfo();
if (TRI.isGeneralPurposeRegister(MF, Reg)) {
- BuildMI(MBB, Iter, DL, get(AArch64::MOVZXi), Reg).addImm(0).addImm(0);
+ BuildMI(MBB, Iter, DL, get(AArch64::MOVZXi), Reg)
+ .addImm(0)
+ .addImm(0);
} else if (STI.hasSVE()) {
- BuildMI(MBB, Iter, DL, get(AArch64::DUP_ZI_D), Reg).addImm(0).addImm(0);
+ BuildMI(MBB, Iter, DL, get(AArch64::DUP_ZI_D), Reg)
+ .addImm(0)
+ .addImm(0);
} else {
- BuildMI(MBB, Iter, DL, get(AArch64::MOVIv2d_ns), Reg).addImm(0);
+ BuildMI(MBB, Iter, DL, get(AArch64::MOVIv2d_ns), Reg)
+ .addImm(0);
}
}
diff --git a/llvm/lib/Target/X86/X86InstrInfo.cpp
b/llvm/lib/Target/X86/X86InstrInfo.cpp
index 93220c057c02da2..ea776a0f59f3ffb 100644
--- a/llvm/lib/Target/X86/X86InstrInfo.cpp
+++ b/llvm/lib/Target/X86/X86InstrInfo.cpp
@@ -9808,7 +9808,8 @@ void X86InstrInfo::buildClearRegister(Register Reg,
MachineBasicBlock &MBB,
return;
if (TRI.isGeneralPurposeRegister(MF, Reg)) {
- // Convert register to the 32-bit version.
+ // Convert register to the 32-bit version. Both 'movl' and 'xorl' clear the
+ // upper bits of a 64-bit register automagically.
Reg = getX86SubSuperRegister(Reg, 32);
if (NoSideEffects)
>From 25e55a5d888f1977d28f6b4e4d9390add8975df4 Mon Sep 17 00:00:00 2001
From: Bill Wendling <mo...@google.com>
Date: Tue, 26 Sep 2023 12:31:37 -0700
Subject: [PATCH 4/5] Rename 'NoSideEffects' to 'AllowSideEffects' to be
clearer
---
llvm/include/llvm/CodeGen/TargetInstrInfo.h | 6 +++---
llvm/lib/Target/AArch64/AArch64InstrInfo.cpp | 2 +-
llvm/lib/Target/AArch64/AArch64InstrInfo.h | 2 +-
llvm/lib/Target/X86/X86InstrInfo.cpp | 5 +++--
llvm/lib/Target/X86/X86InstrInfo.h | 2 +-
5 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/llvm/include/llvm/CodeGen/TargetInstrInfo.h
b/llvm/include/llvm/CodeGen/TargetInstrInfo.h
index 4d188ed6d082efa..ede4f26ed487afe 100644
--- a/llvm/include/llvm/CodeGen/TargetInstrInfo.h
+++ b/llvm/include/llvm/CodeGen/TargetInstrInfo.h
@@ -2088,12 +2088,12 @@ class TargetInstrInfo : public MCInstrInfo {
}
/// Insert an architecture-specific instruction to clear a register. If you
- /// need to avoid sideeffects (e.g. XOR on x86), set \p NoSideEffects to \p
- /// true.
+ /// need to avoid sideeffects (e.g. XOR on x86), set \p AllowSideEffects to
+ /// \p false.
virtual void buildClearRegister(Register Reg, MachineBasicBlock &MBB,
MachineBasicBlock::iterator Iter,
DebugLoc &DL,
- bool NoSideEffects = false) const {
+ bool AllowSideEffects = true) const {
llvm_unreachable(
"Target didn't implement TargetInstrInfo::buildClearRegister!");
}
diff --git a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
index 1faf0d73f5aab66..68985d77318d9ab 100644
--- a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
+++ b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
@@ -9067,7 +9067,7 @@ bool AArch64InstrInfo::shouldOutlineFromFunctionByDefault(
void AArch64InstrInfo::buildClearRegister(Register Reg, MachineBasicBlock &MBB,
MachineBasicBlock::iterator Iter,
DebugLoc &DL,
- bool NoSideEffects) const {
+ bool AllowSideEffects) const {
const MachineFunction &MF = *MBB.getParent();
const AArch64Subtarget &STI = MF.getSubtarget<AArch64Subtarget>();
const AArch64RegisterInfo &TRI = *STI.getRegisterInfo();
diff --git a/llvm/lib/Target/AArch64/AArch64InstrInfo.h
b/llvm/lib/Target/AArch64/AArch64InstrInfo.h
index d60b8900ea26123..c0ea5fa4dc65800 100644
--- a/llvm/lib/Target/AArch64/AArch64InstrInfo.h
+++ b/llvm/lib/Target/AArch64/AArch64InstrInfo.h
@@ -328,7 +328,7 @@ class AArch64InstrInfo final : public AArch64GenInstrInfo {
void buildClearRegister(Register Reg, MachineBasicBlock &MBB,
MachineBasicBlock::iterator Iter, DebugLoc &DL,
- bool NoSideEffects = false) const override;
+ bool AllowSideEffects = true) const override;
/// Returns the vector element size (B, H, S or D) of an SVE opcode.
uint64_t getElementSizeForOpcode(unsigned Opc) const;
diff --git a/llvm/lib/Target/X86/X86InstrInfo.cpp
b/llvm/lib/Target/X86/X86InstrInfo.cpp
index ea776a0f59f3ffb..8e42ef54b67ac11 100644
--- a/llvm/lib/Target/X86/X86InstrInfo.cpp
+++ b/llvm/lib/Target/X86/X86InstrInfo.cpp
@@ -9798,7 +9798,8 @@ X86InstrInfo::insertOutlinedCall(Module &M,
MachineBasicBlock &MBB,
void X86InstrInfo::buildClearRegister(Register Reg, MachineBasicBlock &MBB,
MachineBasicBlock::iterator Iter,
- DebugLoc &DL, bool NoSideEffects) const {
+ DebugLoc &DL,
+ bool AllowSideEffects) const {
const MachineFunction &MF = *MBB.getParent();
const X86Subtarget &ST = MF.getSubtarget<X86Subtarget>();
const TargetRegisterInfo &TRI = getRegisterInfo();
@@ -9812,7 +9813,7 @@ void X86InstrInfo::buildClearRegister(Register Reg,
MachineBasicBlock &MBB,
// upper bits of a 64-bit register automagically.
Reg = getX86SubSuperRegister(Reg, 32);
- if (NoSideEffects)
+ if (!AllowSideEffects)
// XOR affects flags, so use a MOV instead.
BuildMI(MBB, Iter, DL, get(X86::MOV32ri), Reg).addImm(0);
else
diff --git a/llvm/lib/Target/X86/X86InstrInfo.h
b/llvm/lib/Target/X86/X86InstrInfo.h
index a5ad618ef4b6415..7552410c8d5799e 100644
--- a/llvm/lib/Target/X86/X86InstrInfo.h
+++ b/llvm/lib/Target/X86/X86InstrInfo.h
@@ -575,7 +575,7 @@ class X86InstrInfo final : public X86GenInstrInfo {
void buildClearRegister(Register Reg, MachineBasicBlock &MBB,
MachineBasicBlock::iterator Iter, DebugLoc &DL,
- bool NoSideEffects = false) const override;
+ bool AllowSideEffects = true) const override;
bool verifyInstruction(const MachineInstr &MI,
StringRef &ErrInfo) const override;
>From 24f539243ab62b03e70d9d17c45dff125767ba4d Mon Sep 17 00:00:00 2001
From: Bill Wendling <mo...@google.com>
Date: Wed, 27 Sep 2023 11:17:25 -0700
Subject: [PATCH 5/5] Comment an example of side effects we want to avoid.
---
llvm/include/llvm/CodeGen/TargetInstrInfo.h | 4 ++--
llvm/lib/Target/AArch64/AArch64InstrInfo.cpp | 10 +++++-----
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/llvm/include/llvm/CodeGen/TargetInstrInfo.h
b/llvm/include/llvm/CodeGen/TargetInstrInfo.h
index ede4f26ed487afe..7a1c44b7211392f 100644
--- a/llvm/include/llvm/CodeGen/TargetInstrInfo.h
+++ b/llvm/include/llvm/CodeGen/TargetInstrInfo.h
@@ -2088,8 +2088,8 @@ class TargetInstrInfo : public MCInstrInfo {
}
/// Insert an architecture-specific instruction to clear a register. If you
- /// need to avoid sideeffects (e.g. XOR on x86), set \p AllowSideEffects to
- /// \p false.
+ /// need to avoid sideeffects (e.g. avoid XOR on x86, which sets EFLAGS), set
+ /// \p AllowSideEffects to \p false.
virtual void buildClearRegister(Register Reg, MachineBasicBlock &MBB,
MachineBasicBlock::iterator Iter,
DebugLoc &DL,
diff --git a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
index 68985d77318d9ab..f4b903330ab2a3f 100644
--- a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
+++ b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
@@ -9074,15 +9074,15 @@ void AArch64InstrInfo::buildClearRegister(Register Reg,
MachineBasicBlock &MBB,
if (TRI.isGeneralPurposeRegister(MF, Reg)) {
BuildMI(MBB, Iter, DL, get(AArch64::MOVZXi), Reg)
- .addImm(0)
- .addImm(0);
+ .addImm(0)
+ .addImm(0);
} else if (STI.hasSVE()) {
BuildMI(MBB, Iter, DL, get(AArch64::DUP_ZI_D), Reg)
- .addImm(0)
- .addImm(0);
+ .addImm(0)
+ .addImm(0);
} else {
BuildMI(MBB, Iter, DL, get(AArch64::MOVIv2d_ns), Reg)
- .addImm(0);
+ .addImm(0);
}
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
