[CodeGen] Get rid of incorrect std template specializations
#160804
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-backend-hexagon Author: A. Jiang (frederick-vs-ja) ChangesThis patch renames comparators
The original specializations don't satisfy the requirements for the original Fixes #160684. Full diff: https://github.com/llvm/llvm-project/pull/160804.diff 6 Files Affected:
diff --git a/llvm/include/llvm/CodeGen/RDFGraph.h b/llvm/include/llvm/CodeGen/RDFGraph.h
index 8a93afbcb5491..6bb6033a8a2f2 100644
--- a/llvm/include/llvm/CodeGen/RDFGraph.h
+++ b/llvm/include/llvm/CodeGen/RDFGraph.h
@@ -447,7 +447,7 @@ struct NodeAllocator {
AllocatorTy MemPool;
};
-using RegisterSet = std::set<RegisterRef>;
+using RegisterSet = std::set<RegisterRef, RegisterRefLess>;
struct TargetOperandInfo {
TargetOperandInfo(const TargetInstrInfo &tii) : TII(tii) {}
diff --git a/llvm/include/llvm/CodeGen/RDFRegisters.h b/llvm/include/llvm/CodeGen/RDFRegisters.h
index 4a9a4063c9e83..82027cad53bdb 100644
--- a/llvm/include/llvm/CodeGen/RDFRegisters.h
+++ b/llvm/include/llvm/CodeGen/RDFRegisters.h
@@ -199,6 +199,33 @@ struct PhysicalRegisterInfo {
std::vector<AliasInfo> AliasInfos;
};
+struct RegisterRefEqualTo {
+ constexpr RegisterRefEqualTo(const llvm::rdf::PhysicalRegisterInfo &pri)
+ : PRI(&pri) {}
+
+ bool operator()(llvm::rdf::RegisterRef A, llvm::rdf::RegisterRef B) const {
+ return PRI->equal_to(A, B);
+ }
+
+private:
+ // Make it a pointer just in case. See comment in `RegisterRefLess` below.
+ const llvm::rdf::PhysicalRegisterInfo *PRI;
+};
+
+struct RegisterRefLess {
+ constexpr RegisterRefLess(const llvm::rdf::PhysicalRegisterInfo &pri)
+ : PRI(&pri) {}
+
+ bool operator()(llvm::rdf::RegisterRef A, llvm::rdf::RegisterRef B) const {
+ return PRI->less(A, B);
+ }
+
+private:
+ // Make it a pointer because apparently some versions of MSVC use std::swap
+ // on the comparator object.
+ const llvm::rdf::PhysicalRegisterInfo *PRI;
+};
+
struct RegisterAggr {
RegisterAggr(const PhysicalRegisterInfo &pri)
: Units(pri.getTRI().getNumRegUnits()), PRI(pri) {}
@@ -334,18 +361,6 @@ template <> struct hash<llvm::rdf::RegisterAggr> {
}
};
-template <> struct equal_to<llvm::rdf::RegisterRef> {
- constexpr equal_to(const llvm::rdf::PhysicalRegisterInfo &pri) : PRI(&pri) {}
-
- bool operator()(llvm::rdf::RegisterRef A, llvm::rdf::RegisterRef B) const {
- return PRI->equal_to(A, B);
- }
-
-private:
- // Make it a pointer just in case. See comment in `less` below.
- const llvm::rdf::PhysicalRegisterInfo *PRI;
-};
-
template <> struct equal_to<llvm::rdf::RegisterAggr> {
bool operator()(const llvm::rdf::RegisterAggr &A,
const llvm::rdf::RegisterAggr &B) const {
@@ -353,23 +368,10 @@ template <> struct equal_to<llvm::rdf::RegisterAggr> {
}
};
-template <> struct less<llvm::rdf::RegisterRef> {
- constexpr less(const llvm::rdf::PhysicalRegisterInfo &pri) : PRI(&pri) {}
-
- bool operator()(llvm::rdf::RegisterRef A, llvm::rdf::RegisterRef B) const {
- return PRI->less(A, B);
- }
-
-private:
- // Make it a pointer because apparently some versions of MSVC use std::swap
- // on the std::less specialization.
- const llvm::rdf::PhysicalRegisterInfo *PRI;
-};
-
} // namespace std
namespace llvm::rdf {
-using RegisterSet = std::set<RegisterRef, std::less<RegisterRef>>;
+using RegisterSet = std::set<RegisterRef, RegisterRefLess>;
} // namespace llvm::rdf
#endif // LLVM_CODEGEN_RDFREGISTERS_H
diff --git a/llvm/lib/CodeGen/RDFLiveness.cpp b/llvm/lib/CodeGen/RDFLiveness.cpp
index 318422b46e811..f6afe80422dda 100644
--- a/llvm/lib/CodeGen/RDFLiveness.cpp
+++ b/llvm/lib/CodeGen/RDFLiveness.cpp
@@ -652,8 +652,9 @@ void Liveness::computePhiInfo() {
// defs, cache the result of subtracting these defs from a given register
// ref.
using RefHash = std::hash<RegisterRef>;
- using RefEqual = std::equal_to<RegisterRef>;
- using SubMap = std::unordered_map<RegisterRef, RegisterRef>;
+ using RefEqual = RegisterRefEqualTo;
+ using SubMap = std::unordered_map<RegisterRef, RegisterRef,
+ std::hash<RefEqual>, RefEqual>;
std::unordered_map<RegisterAggr, SubMap> Subs;
auto ClearIn = [](RegisterRef RR, const RegisterAggr &Mid, SubMap &SM) {
if (Mid.empty())
@@ -868,7 +869,7 @@ void Liveness::computeLiveIns() {
std::vector<RegisterRef> LV;
for (const MachineBasicBlock::RegisterMaskPair &LI : B.liveins())
LV.push_back(RegisterRef(LI.PhysReg, LI.LaneMask));
- llvm::sort(LV, std::less<RegisterRef>(PRI));
+ llvm::sort(LV, RegisterRefLess(PRI));
dbgs() << printMBBReference(B) << "\t rec = {";
for (auto I : LV)
dbgs() << ' ' << Print(I, DFG);
@@ -878,7 +879,7 @@ void Liveness::computeLiveIns() {
LV.clear();
for (RegisterRef RR : LiveMap[&B].refs())
LV.push_back(RR);
- llvm::sort(LV, std::less<RegisterRef>(PRI));
+ llvm::sort(LV, RegisterRefLess(PRI));
dbgs() << "\tcomp = {";
for (auto I : LV)
dbgs() << ' ' << Print(I, DFG);
diff --git a/llvm/lib/Target/Hexagon/RDFCopy.cpp b/llvm/lib/Target/Hexagon/RDFCopy.cpp
index fafdad08909dd..3b1d3bd89680b 100644
--- a/llvm/lib/Target/Hexagon/RDFCopy.cpp
+++ b/llvm/lib/Target/Hexagon/RDFCopy.cpp
@@ -108,7 +108,7 @@ bool CopyPropagation::scanBlock(MachineBasicBlock *B) {
for (NodeAddr<InstrNode*> IA : BA.Addr->members(DFG)) {
if (DFG.IsCode<NodeAttrs::Stmt>(IA)) {
NodeAddr<StmtNode*> SA = IA;
- EqualityMap EM(std::less<RegisterRef>(DFG.getPRI()));
+ EqualityMap EM(RegisterRefLess(DFG.getPRI()));
if (interpretAsCopy(SA.Addr->getCode(), EM))
recordCopy(SA, EM);
}
diff --git a/llvm/lib/Target/Hexagon/RDFCopy.h b/llvm/lib/Target/Hexagon/RDFCopy.h
index e4fb89892831d..92b2c65982655 100644
--- a/llvm/lib/Target/Hexagon/RDFCopy.h
+++ b/llvm/lib/Target/Hexagon/RDFCopy.h
@@ -25,8 +25,8 @@ class MachineInstr;
namespace rdf {
struct CopyPropagation {
- CopyPropagation(DataFlowGraph &dfg) : MDT(dfg.getDT()), DFG(dfg),
- RDefMap(std::less<RegisterRef>(DFG.getPRI())) {}
+ CopyPropagation(DataFlowGraph &dfg)
+ : MDT(dfg.getDT()), DFG(dfg), RDefMap(RegisterRefLess(DFG.getPRI())) {}
virtual ~CopyPropagation() = default;
@@ -35,7 +35,7 @@ namespace rdf {
bool trace() const { return Trace; }
DataFlowGraph &getDFG() { return DFG; }
- using EqualityMap = std::map<RegisterRef, RegisterRef>;
+ using EqualityMap = std::map<RegisterRef, RegisterRef, RegisterRefLess>;
virtual bool interpretAsCopy(const MachineInstr *MI, EqualityMap &EM);
private:
@@ -45,7 +45,7 @@ namespace rdf {
bool Trace = false;
// map: register -> (map: stmt -> reaching def)
- std::map<RegisterRef,std::map<NodeId,NodeId>> RDefMap;
+ std::map<RegisterRef, std::map<NodeId, NodeId>, RegisterRefLess> RDefMap;
// map: statement -> (map: dst reg -> src reg)
std::map<NodeId, EqualityMap> CopyMap;
std::vector<NodeId> Copies;
diff --git a/llvm/unittests/CodeGen/TypeTraitsTest.cpp b/llvm/unittests/CodeGen/TypeTraitsTest.cpp
index dde86280cff6a..5c0a74d3e2c0f 100644
--- a/llvm/unittests/CodeGen/TypeTraitsTest.cpp
+++ b/llvm/unittests/CodeGen/TypeTraitsTest.cpp
@@ -6,13 +6,16 @@
//
//===----------------------------------------------------------------------===//
+#include "llvm/CodeGen/RDFRegisters.h"
#include "llvm/CodeGen/RegisterPressure.h"
#include "llvm/CodeGen/ScheduleDAG.h"
#include "llvm/CodeGen/SelectionDAGNodes.h"
#include "llvm/CodeGen/SlotIndexes.h"
#include "llvm/CodeGen/TargetPassConfig.h"
#include "gtest/gtest.h"
+#include <functional>
#include <type_traits>
+#include <utility>
using namespace llvm;
@@ -23,3 +26,29 @@ static_assert(std::is_trivially_copyable_v<SDValue>, "trivially copyable");
static_assert(std::is_trivially_copyable_v<SlotIndex>, "trivially copyable");
static_assert(std::is_trivially_copyable_v<IdentifyingPassPtr>,
"trivially copyable");
+
+// https://llvm.org/PR105169
+// Verify that we won't accidently specialize std::less and std::equal_to in a
+// wrong way. See also [namespace.std]/5.
+template <class Fn> constexpr bool CheckStdCmpRequirements() {
+ // std::less and std::equal_to are literal, default constructible, and
+ // copyable classes.
+ Fn f1;
+ auto f2 = f1;
+ auto f3 = std::move(f2);
+ f2 = f3;
+ f2 = std::move(f3);
+
+ // Properties held on all known implementations, although not guaranteed by
+ // the standard.
+ static_assert(std::is_empty_v<Fn>);
+ static_assert(std::is_trivially_default_constructible_v<Fn>);
+ static_assert(std::is_trivially_copyable_v<Fn>);
+
+ return true;
+}
+
+static_assert(CheckStdCmpRequirements<std::less<rdf::RegisterRef>>(),
+ "same as the original template");
+static_assert(CheckStdCmpRequirements<std::equal_to<rdf::RegisterRef>>(),
+ "same as the original template");
|
frederick-vs-ja
requested review from
philnik777,
kparzysz-quic and
kparzysz
frederick-vs-ja
commented
Sep 26, 2025
Comment on lines
364
to
369
| template <> struct equal_to<llvm::rdf::RegisterAggr> { | ||
| bool operator()(const llvm::rdf::RegisterAggr &A, | ||
| const llvm::rdf::RegisterAggr &B) const { | ||
| return A == B; | ||
| } | ||
| }; |
There was a problem hiding this comment.
This full specialization is correct but probably meaningless. Perhaps we should remove it in a later PR.
|
This change makes sense to me. @iajbar or @aankit-quic please take a look. |
frederick-vs-ja
force-pushed
the
llvm-no-cmp-spec
branch
from
8fe6d73 to
844d278
Compare
|
I approve this change. The stateful specialization of |
|
You should clarify which requirement is violated. Edit: This change is ok to me, but I'd like to know what the current code violates. |
This patch renames comparators - from `std::equal_to<llvm::rdf::RegisterRef>` to `llvm::rdf::RegisterRefEqualTo`, and - from `std::less<llvm::rdf::RegisterRef>` to `llvm::rdf::RegisterRefLess`. The original specializations don't satisfy the requirements for the original `std` templates being stateful and non-default-constructible, so they make the program have UB due to C++17 [namespace.std]/2, C++20/23 [namespace.std]/5. > A program may explicitly instantiate a class template defined in the standard library only if the declaration > - depends on the name of at least one program-defined type, and > - the instantiation meets the standard library requirements for the original template.
frederick-vs-ja
force-pushed
the
llvm-no-cmp-spec
branch
from
844d278 to
4bed796
Compare
|
@kparzysz Thanks. The requirements was in [namespace.std]/2 in C++17. I've updated the PR message and comments to make the reason of changes clearer. |
kparzysz
approved these changes
Sep 28, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/140/builds/31501 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/95/builds/18471 Here is the relevant piece of the build log for the reference |
|
Sadly, it fails in our CI: |
|
@pawosm-arm #161085 Should have fixed CI failure due to uninitialized variables. |
Yup, it just passed through it and no sign of failure. |
mahesh-attarde
pushed a commit
to mahesh-attarde/llvm-project
that referenced
this pull request
Oct 3, 2025
…60804) This patch renames comparators - from `std::equal_to<llvm::rdf::RegisterRef>` to `llvm::rdf::RegisterRefEqualTo`, and - from `std::less<llvm::rdf::RegisterRef>` to `llvm::rdf::RegisterRefLess`. The original specializations don't satisfy the requirements for the original `std` templates by being stateful and non-default-constructible, so they make the program have UB due to C++17 [namespace.std]/2, C++20/23 [namespace.std]/5. > A program may explicitly instantiate a class template defined in the standard library only if the declaration > - depends on the name of at least one program-defined type, and > - the instantiation meets the standard library requirements for the original template.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch renames comparators
std::equal_to<llvm::rdf::RegisterRef>tollvm::rdf::RegisterRefEqualTo, andstd::less<llvm::rdf::RegisterRef>tollvm::rdf::RegisterRefLess.The original specializations don't satisfy the requirements for the original
stdtemplates by being stateful and non-default-constructible, so they make the program have UB due to C++17 [namespace.std]/2, C++20/23 [namespace.std]/5.Fixes #160684.