https://gcc.gnu.org/bugzilla/show_bug.cgi?id=122455
--- Comment #13 from Soumya AR <soumyaa at gcc dot gnu.org> ---
Sorry for the insane delay on this, I had forgotten about this bug. I took
another look at it and the actual reason for the slowdown is more complicated.
For some context:
Source::Peek/Skip have two likely targets in different files:
ByteArraySource (in snappy-sinksource.cc)
SnappyIOVecReader (in snappy.cc)
With max-devirt-targets=3, GCC tries to speculate on both, but drops
ByteArraySource because its body isn't in the same TU being processed. It ends
up speculating only on SnappyIOVecReader, while in the regressing benchmarks
the target is always ByteArraySource.
But as Richi suggested, the guard is well predicted. Branch mispredicts are
close to zero in both builds, so the wrong guess itself should not cause a
slowdown.
The following source:
```
const char* ip = reader_->Peek(&n); // read reader_; Peek writes to n
if (n == 0) return false; // read n
...
reader_->Skip(1); // read reader_ again
```
Old code (no speculation):
```
r = decompressor.reader_;
ip = r->Peek(&n); // opaque call
if (n == 0) goto exit; // read n
r2 = decompressor.reader_; // read reader_ after the check
r2->Skip(1);
```
New code, after speculation (before PRE):
```
r = decompressor.reader_;
if (r->Peek == &SnappyIOVecReader::Peek) {
n = r->curr_size_remaining_; // path A: inlined
ip = r->curr_pos_;
} else {
ip = r->Peek(&n); // path B: opaque call
}
if (n == 0) goto exit; // read n
r2 = decompressor.reader_; // read reader_
r2->Skip(1);
```
Path A has no call, so GCC knows the value of n and knows decompressor.reader_
is still r.
Path B makes an opaque call and decompressor's address escapes elsewhere so
decompressor.reader_ has to be re-read.
The reads after the join are therefore redundant on path A only.
After PRE:
```
r = decompressor.reader_;
if (r->Peek == &SnappyIOVecReader::Peek) {
n_val = r->curr_size_remaining_; // path A: no reads needed
ip = r->curr_pos_;
r2 = r;
} else {
ip = r->Peek(&n); // path B
n_val = n; // read n
r2 = decompressor.reader_; // read reader_
}
if (n_val == 0) goto exit;
r2->Skip(1);
```
n and decompressor.reader_ can now be fused into one ldp (which couldn't happen
earlier because of the n == 0 check in between).
But, ByteArraySource::Peek does an 8 byte write to n. Earlier, the subsequent 8
byte load of n could benefit from store to load forwarding.
But in the newer code, the 16 byte ldp is only half covered and can't be
forwarded.
Splitting just that one ldp into two ldrs, or disabling peephole2, recovers
most of the regression. But this seems too specific to handle I suppose...
Should I close this bug then?
