[SIL] Perf: Cache basic block during SILSuccessor iteration #16869
Conversation
|
@swift-ci please smoke test |
include/swift/SIL/SILSuccessor.h
Outdated
|
|
||
| bool operator==(pred_iterator I2) const { return Cur == I2.Cur; } | ||
| bool operator!=(pred_iterator I2) const { return Cur != I2.Cur; } | ||
|
|
||
| pred_iterator &operator++() { | ||
| assert(Cur && "Trying to advance past end"); | ||
| // No need to assert(Cur). Dereferencing nullptr always crashes. |
There was a problem hiding this comment.
We want a nice error message here. I would suggest actually making Cur a NullablePtr to make it explicit.
There was a problem hiding this comment.
I never understood the "crash intentionally (a.k.a. "assert") before an implicit crash" convention. What problem is this solving?
There was a problem hiding this comment.
If you are looking at why something crashed, it provides a really nice error message without you having to load up the debugger.
include/swift/SIL/SILSuccessor.h
Outdated
| pred_iterator operator++(int) { | ||
| pred_iterator copy = *this; | ||
| ++*this; | ||
| pred_iterator operator+(size_t distance) const { |
There was a problem hiding this comment.
Do we really need to use a size_t here? I thought we were generally using int in such cases, no?
There was a problem hiding this comment.
Unless I missed something, the iterator is unidirectional, so an unsigned type would be appropriate here, right?
There was a problem hiding this comment.
I was just talking about the usage of size_t. We generally use unsigned for these cases.
include/swift/SIL/SILSuccessor.h
Outdated
| @@ -87,33 +87,45 @@ class SILSuccessor { | |||
| class pred_iterator { | |||
| SILSuccessor *Cur; | |||
|
|
|||
| // Cache the basic block to avoid repeated pointer chasing. | |||
| SILBasicBlock *BB; | |||
There was a problem hiding this comment.
Two things:
- Can you use Block instead of BB for this name.
- I am worried about having to call cacheBasicBlock everywhere. It is an extra step that someone may forget. At least I think it is important to have some sort of assert when you access the block to show that the block is the correct block, even though I would prefer if there was an algebraic way that the code could guarantee that BB will be changed if Cur is changed. It seems almost silly though to say wrap the SILSuccessor field in another struct that makes sure that BB is always updated.
There was a problem hiding this comment.
Sure, I can change the name.
I think if the maintainer of the iterator is comfortable with how C++ iterators work, then the calls to cacheBasicBlock will very limited in scope. That being said, if you don't like this tradeoff, then I'll just abandon the pull request. (I'm not invested in this pull request. It just felt like a "freebie" while I was hacking on some other stuff.)
include/swift/SIL/SILInstruction.h
Outdated
| @@ -8094,6 +8094,11 @@ inline EnumElementDecl **SelectEnumInstBase::getEnumElementDeclStorage() { | |||
| llvm_unreachable("Unhandled SelectEnumInstBase subclass"); | |||
| } | |||
|
|
|||
| inline void SILSuccessor::pred_iterator::cacheBasicBlock() { | |||
| // No need to assert(Cur). Dereferencing nullptr always crashes. | |||
There was a problem hiding this comment.
While it may crash, providing a nice error message when we hit that condition can be useful when triaging bugs and also provides a nice guide for the reader of the code that a pointer is intended to be non-null. Can you put that back in?
'operator*' in this patch becomes a hot spot in some scenarios. For example: an enum with an unusually large number of cases.
davezarzycki
force-pushed
the
cache_bb_during_SILSuccessor_iteration
branch
from
b741473 to
52fd7cb
Compare
|
Ping |
|
SGTM |
'operator*' in this patch becomes a hot spot in some scenarios. For example: an enum with an unusually large number of cases.