Commit 4029908d authored by Nathan Bronson's avatar Nathan Bronson Committed by Facebook Github Bot

fix elision of iter advance in F14 erase with unused return value

Summary:
D8395569 added a prefetch instruction as part of iterator advance
for F14Value and F14Node maps.  This caused the work to advance the
iterator returned by erase to no longer be recognized as dead code by the
GCC optimizer.  This diff separates out advance() and advanceLikelyDead()
methods, avoiding prefetch in the latter case.

Reviewed By: shixiao

Differential Revision: D8656038

fbshipit-source-id: 86cecf40e8622f815d34a98709b90d6ef73dd618
parent a99ccb8b
...@@ -572,15 +572,17 @@ class F14BasicMap { ...@@ -572,15 +572,17 @@ class F14BasicMap {
// work of itemPos.advance() if our return value is discarded. // work of itemPos.advance() if our return value is discarded.
auto itemPos = table_.unwrapIter(pos); auto itemPos = table_.unwrapIter(pos);
table_.eraseIter(itemPos); table_.eraseIter(itemPos);
itemPos.advance(); itemPos.advanceLikelyDead();
return table_.makeIter(itemPos); return table_.makeIter(itemPos);
} }
// This form avoids ambiguity when key_type has a templated constructor // This form avoids ambiguity when key_type has a templated constructor
// that accepts const_iterator // that accepts const_iterator
iterator erase(iterator pos) { iterator erase(iterator pos) {
table_.eraseIter(table_.unwrapIter(pos)); auto itemPos = table_.unwrapIter(pos);
return ++pos; table_.eraseIter(itemPos);
itemPos.advanceLikelyDead();
return table_.makeIter(itemPos);
} }
iterator erase(const_iterator first, const_iterator last) { iterator erase(const_iterator first, const_iterator last) {
......
...@@ -432,11 +432,13 @@ class F14BasicSet { ...@@ -432,11 +432,13 @@ class F14BasicSet {
template <typename BeforeDestroy> template <typename BeforeDestroy>
FOLLY_ALWAYS_INLINE iterator FOLLY_ALWAYS_INLINE iterator
eraseInto(const_iterator pos, BeforeDestroy&& beforeDestroy) { eraseInto(const_iterator pos, BeforeDestroy&& beforeDestroy) {
table_.eraseIterInto(table_.unwrapIter(pos), beforeDestroy); auto itemPos = table_.unwrapIter(pos);
table_.eraseIterInto(itemPos, beforeDestroy);
// If we are inlined then gcc and clang can optimize away all of the // If we are inlined then gcc and clang can optimize away all of the
// work of ++pos if the caller discards it. // work of ++pos if the caller discards it.
return ++pos; itemPos.advanceLikelyDead();
return table_.makeIter(itemPos);
} }
template <typename BeforeDestroy> template <typename BeforeDestroy>
......
...@@ -885,7 +885,7 @@ class F14ItemIter { ...@@ -885,7 +885,7 @@ class F14ItemIter {
folly::assume(itemPtr_ != nullptr); folly::assume(itemPtr_ != nullptr);
} }
FOLLY_ALWAYS_INLINE void advance() { FOLLY_ALWAYS_INLINE void advanceImpl(bool checkEof, bool likelyDead) {
auto c = chunk(); auto c = chunk();
// common case is packed entries // common case is packed entries
...@@ -918,16 +918,22 @@ class F14ItemIter { ...@@ -918,16 +918,22 @@ class F14ItemIter {
// //
// https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82776 has the bug we // https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82776 has the bug we
// filed about the issue. while (true) { // filed about the issue. while (true) {
for (std::size_t i = 1; i != 0; ++i) { for (std::size_t i = 1; !likelyDead || i != 0; ++i) {
if (checkEof) {
// exhausted the current chunk // exhausted the current chunk
if (UNLIKELY(c->eof())) { if (UNLIKELY(c->eof())) {
FOLLY_SAFE_DCHECK(index_ == 0, ""); FOLLY_SAFE_DCHECK(index_ == 0, "");
itemPtr_ = nullptr; itemPtr_ = nullptr;
return; return;
} }
} else {
FOLLY_SAFE_DCHECK(!c->eof(), "");
}
--c; --c;
auto last = c->lastOccupied(); auto last = c->lastOccupied();
if (checkEof && !likelyDead) {
prefetchAddr(&*c - 1); prefetchAddr(&*c - 1);
}
if (LIKELY(last.hasIndex())) { if (LIKELY(last.hasIndex())) {
index_ = last.index(); index_ = last.index();
itemPtr_ = std::pointer_traits<ItemPtr>::pointer_to(c->item(index_)); itemPtr_ = std::pointer_traits<ItemPtr>::pointer_to(c->item(index_));
...@@ -936,31 +942,16 @@ class F14ItemIter { ...@@ -936,31 +942,16 @@ class F14ItemIter {
} }
} }
// precheckedAdvance requires knowledge that the current iterator
// position isn't the last item
void precheckedAdvance() { void precheckedAdvance() {
auto c = chunk(); advanceImpl(false, false);
// common case is packed entries
while (index_ > 0) {
--index_;
--itemPtr_;
if (LIKELY(c->occupied(index_))) {
return;
}
} }
while (true) { FOLLY_ALWAYS_INLINE void advance() {
// exhausted the current chunk advanceImpl(true, false);
FOLLY_SAFE_DCHECK(!c->eof(), "");
--c;
auto last = c->lastOccupied();
if (LIKELY(last.hasIndex())) {
index_ = last.index();
itemPtr_ = std::pointer_traits<ItemPtr>::pointer_to(c->item(index_));
return;
}
} }
FOLLY_ALWAYS_INLINE void advanceLikelyDead() {
advanceImpl(true, true);
} }
ChunkPtr chunk() const { ChunkPtr chunk() const {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment