david-xl wrote:
@MatzeB ,Our internal release testing have seen lots of very large regressions
for tests without PGO or with XFDO only. While I agree this patch is the right
way to go and the good performance (without PGO) with oldBFI is somewhat by
chance, it is still better not to regress th
david-xl wrote:
> And digging even deeper:
>
> * FWIW I noticed that I only used `clang -c` as benchmark previously, should
> have used `clang -c -O3` resulting in this:
>
> ```
> Old-BFI: insn: 37,821,687,947 (baseline)
> New-BFI: insn: 38,133,312,923 +0.82%
> Old-BFI, no-
MatzeB wrote:
And digging even deeper:
- FWIW I noticed that I only used `clang -c` as benchmark previously, should
have used `clang -c -O3` resulting in this:
```
Old-BFI: insn: 37,821,687,947 (baseline)
New-BFI: insn: 38,133,312,923 +0.82%
Old-BFI, no-cold: insn: 37,423,365
MatzeB wrote:
Not sure how this change could trigger leaks. Changing BFI here should mostly
effect inlining decisions, basic block placement, register allocation. But it
shouldn't change program behavior or memory operations...
Will try to reproduce this test on my machine, to see if there's a
MatzeB wrote:
Seems this got introduced in https://reviews.llvm.org/D34312 with the rough
idea that we shouldn't inline into parts of the code that
`_builtin_expect(...)` deems unlikely. Which makes sense when you say it
express it like this, but I guess numeric thresholds can go wrong...
htt
david-xl wrote:
Agree that deciding coldness based on local entry frequency (2%) is a bad idea
though.
https://github.com/llvm/llvm-project/pull/66285
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listi
dexonsmith wrote:
Not entirely accidental. When BPI/BFI first landed it was heavily profiled to
be sure it didn't pessimize non-PGO code. I don't see why we'd suddenly be okay
with pessimizing it.
Under 2% isn't hard to hit for hot path code. Lots of functions will have
strings of early exit
david-xl wrote:
The old code of not marking them as cold works also just by accident though.
https://github.com/llvm/llvm-project/pull/66285
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-com
dexonsmith wrote:
Seems awkward to pessimize as "cold" when there's no real data (no PGO, no
other marking). What happens if you change the cold call threshold to 0% when
there's no PGO data? (I.e., never assume a call is cold without actual evidence)
https://github.com/llvm/llvm-project/pull/
MatzeB wrote:
Also note that this 2% cold-code threshold is only applied in situation where
no PGO data is available. So maybe we should just ignore this, given that
without PGO data it just is often impossible for the compiler to make good
inlining decisions...
https://github.com/llvm/llvm-p
MatzeB wrote:
I think I traced the clang regression down to
`InlineCostCallAnalyzer::isColdCallSite` returning `true` in more instances. It
previously did not do that because of what feels more like an accidental loss
of precision:
The example I analyzed starts out as a single-block function
MatzeB wrote:
So not sure if there is an easy fix for the regression.
We should probably have some logic that scales all BFI values when inlining to
better accomodate the new range of frequencies so we loose less precision by
accident. Though that won't help with the regression at hand as that
david-xl wrote:
Yes -- the revert can wait until more data is available. I agree that it should
help performance in theory.
https://github.com/llvm/llvm-project/pull/66285
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/
WenleiHe wrote:
> This is concerning. Can this be reverted for now and we can help with some
> internal performance testing. @xur-llvm
Internally, with this change, on a few large workloads, we saw ~0.2-0.5%
performance improvements, all with 2-3% .text size reduction at the same time.
Our te
MatzeB wrote:
Oh so this isn't even a PGO enabled build, interesting...
https://github.com/llvm/llvm-project/pull/66285
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
MatzeB wrote:
The internal services I tried here tended to slightly improve or stay neutral.
We can revert, but could you please share some details on how your clang built
/ tested so I have a chance to address things and bring these changes back?
https://github.com/llvm/llvm-project/pull/6628
david-xl wrote:
This is concerning. Can this be reverted for now and we can help with some
internal performance testing. @xur-llvm
https://github.com/llvm/llvm-project/pull/66285
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.l
17 matches
Mail list logo
