Skip to content

Commit 0541f2a

Browse files
authored
[SYCL][Graph] Fix GCC 11.4 warning/issues in graph_memory_pool (#18583)
- Rework code to avoid false positive uninitialized optional warning in GCC 11.4 - Uncomment early return code from tryReuseExistingAllocation - Fix resetting last free node on temporary instead of value in MAllocations - Also update codeowners for graphs to include graph_memory_pool.hpp/cpp
1 parent 597709b commit 0541f2a

File tree

2 files changed

+23
-25
lines changed

2 files changed

+23
-25
lines changed

.github/CODEOWNERS

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,8 @@ libdevice/nativecpu* @intel/dpcpp-nativecpu-reviewers
165165
sycl/include/sycl/ext/oneapi/experimental/graph.hpp @intel/sycl-graphs-reviewers
166166
sycl/source/detail/graph_impl.cpp @intel/sycl-graphs-reviewers
167167
sycl/source/detail/graph_impl.hpp @intel/sycl-graphs-reviewers
168+
sycl/source/detail/graph_memory_pool.hpp @intel/sycl-graphs-reviewers
169+
sycl/source/detail/graph_memory_pool.cpp @intel/sycl-graphs-reviewers
168170
sycl/unittests/Extensions/CommandGraph/ @intel/sycl-graphs-reviewers
169171
sycl/test-e2e/Graph @intel/sycl-graphs-reviewers
170172
sycl/doc/design/CommandGraph.md @intel/sycl-graphs-reviewers

sycl/source/detail/graph_memory_pool.cpp

Lines changed: 21 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -90,9 +90,9 @@ graph_mem_pool::tryReuseExistingAllocation(
9090
const std::vector<std::shared_ptr<node_impl>> &DepNodes) {
9191
// If we have no dependencies this is a no-op because allocations must connect
9292
// to a free node for reuse to be possible.
93-
// if (DepNodes.empty()) {
94-
// return std::nullopt;
95-
// }
93+
if (DepNodes.empty()) {
94+
return std::nullopt;
95+
}
9696

9797
std::vector<alloc_info> CompatibleAllocs;
9898
// Compatible allocs can only be as big as MFreeAllocations
@@ -127,26 +127,22 @@ graph_mem_pool::tryReuseExistingAllocation(
127127
NodesToCheck.push(Dep);
128128
}
129129

130-
std::optional<alloc_info> AllocInfo = {};
131-
132130
// Called when traversing over nodes to check if the current node is a free
133131
// node for one of the available allocations. If it is we populate AllocInfo
134132
// with the allocation to be reused.
135133
auto CheckNodeEqual =
136-
[&CompatibleAllocs,
137-
&AllocInfo](const std::shared_ptr<node_impl> &CurrentNode) -> bool {
134+
[&CompatibleAllocs](const std::shared_ptr<node_impl> &CurrentNode)
135+
-> std::optional<alloc_info> {
138136
for (auto &Alloc : CompatibleAllocs) {
139137
const auto &AllocFreeNode = Alloc.LastFreeNode;
140138
// Compare control blocks without having to lock AllocFreeNode to check
141139
// for node equality
142140
if (!CurrentNode.owner_before(AllocFreeNode) &&
143141
!AllocFreeNode.owner_before(CurrentNode)) {
144-
Alloc.LastFreeNode.reset();
145-
AllocInfo = Alloc;
146-
return true;
142+
return Alloc;
147143
}
148144
}
149-
return false;
145+
return std::nullopt;
150146
};
151147

152148
while (!NodesToCheck.empty()) {
@@ -161,11 +157,19 @@ graph_mem_pool::tryReuseExistingAllocation(
161157
// for any of the allocations which are free for reuse. We should not bother
162158
// checking nodes that are not free nodes, so we continue and check their
163159
// predecessors.
164-
if (CurrentNode->MNodeType == node_type::async_free &&
165-
CheckNodeEqual(CurrentNode)) {
166-
// If we found an allocation AllocInfo has already been populated in
167-
// CheckNodeEqual(), so we simply break out of the loop
168-
break;
160+
if (CurrentNode->MNodeType == node_type::async_free) {
161+
std::optional<alloc_info> AllocFound = CheckNodeEqual(CurrentNode);
162+
if (AllocFound) {
163+
// Reset visited nodes tracking
164+
MGraph.resetNodeVisitedEdges();
165+
// Reset last free node for allocation
166+
MAllocations.at(AllocFound.value().Ptr).LastFreeNode.reset();
167+
// Remove found allocation from the free list
168+
MFreeAllocations.erase(std::find(MFreeAllocations.begin(),
169+
MFreeAllocations.end(),
170+
AllocFound.value().Ptr));
171+
return AllocFound;
172+
}
169173
}
170174

171175
// Add CurrentNode predecessors to queue
@@ -176,16 +180,8 @@ graph_mem_pool::tryReuseExistingAllocation(
176180
// Mark node as visited
177181
CurrentNode->MTotalVisitedEdges = 1;
178182
}
179-
// Reset visited nodes tracking
180-
MGraph.resetNodeVisitedEdges();
181-
// If we found an allocation, remove it from the free list.
182-
if (AllocInfo) {
183-
MFreeAllocations.erase(std::find(MFreeAllocations.begin(),
184-
MFreeAllocations.end(),
185-
AllocInfo.value().Ptr));
186-
}
187183

188-
return AllocInfo;
184+
return std::nullopt;
189185
}
190186

191187
void graph_mem_pool::markAllocationAsAvailable(

0 commit comments

Comments
 (0)