[Bug target/86677] popcount builtin detection is breaking some kernel build
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86677 --- Comment #13 from kugan at gcc dot gnu.org --- Author: kugan Date: Mon Nov 12 23:43:56 2018 New Revision: 266039 URL: https://gcc.gnu.org/viewcvs?rev=266039=gcc=rev Log: gcc/ChangeLog: 2018-11-13 Kugan Vivekanandarajah PR middle-end/86677 PR middle-end/87528 * tree-scalar-evolution.c (expression_expensive_p): Make BUILTIN POPCOUNT as expensive when backend does not define it. gcc/testsuite/ChangeLog: 2018-11-13 Kugan Vivekanandarajah PR middle-end/86677 PR middle-end/87528 * g++.dg/tree-ssa/pr86544.C: Run only for target supporting popcount pattern. * gcc.dg/tree-ssa/popcount.c: Likewise. * gcc.dg/tree-ssa/popcount2.c: Likewise. * gcc.dg/tree-ssa/popcount3.c: Likewise. * gcc.target/aarch64/popcount4.c: New test. * lib/target-supports.exp (check_effective_target_popcountl): New. Added: trunk/gcc/testsuite/gcc.target/aarch64/popcount4.c Modified: trunk/gcc/ChangeLog trunk/gcc/testsuite/ChangeLog trunk/gcc/testsuite/g++.dg/tree-ssa/pr86544.C trunk/gcc/testsuite/gcc.dg/tree-ssa/popcount.c trunk/gcc/testsuite/gcc.dg/tree-ssa/popcount2.c trunk/gcc/testsuite/gcc.dg/tree-ssa/popcount3.c trunk/gcc/testsuite/lib/target-supports.exp trunk/gcc/tree-scalar-evolution.c
[Bug target/86677] popcount builtin detection is breaking some kernel build
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86677
--- Comment #12 from Richard Biener ---
(In reply to Martin Liška from comment #11)
> (In reply to Wilco from comment #10)
> > (In reply to Martin Liška from comment #9)
> > > Taking look at
> > > ../drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c file:
> > >
> > > The __builtin_popcount is generated from:
> > >
> > > static int brcmf_setup_wiphybands(struct brcmf_cfg80211_info *cfg)
> > > {
> > > ...
> > > } else {
> > > for (nchain = 0; rxchain; nchain++)
> > >rxchain = rxchain & (rxchain - 1);
> > > }
> > > ...
> > > }
> > >
> > > It started from r262486. I would that in this situation it's maybe
> > > unexpected that a popcount call will be eventually generated. So I'm for
> > > an
> > > option what will disable the pattern recognition.
> >
> > I'd say this is exactly a case where using a faster popcount instruction
> > helps - if available of course.
>
> Yes, but as seen in PR87528, using a libgcc can slow down a SPEC benchmark.
> But it's related to generic tuning.
>
> >
> > Is there any reason -fno-builtin-popcount can't also block automatic
> > generation of popcount?
>
> Do we really have such option?
None that would work in the way intended.
Note that as I said in the referenced thread a fix could boil down to
adjusting expression_expensive_p to return true if there's no optab
for the builtin/IFN and/or adjusting is_inexpensive_builtin in that way
(with wider effect).
Note that number_of_iterations_popcount might as well create IFN_POPCOUNT
for the case there _is_ an optab. I'm not sure whether we only have/allow
the IFN in case there's an optab.
So let's discuss the efficiency issue in PR87528. PR86677 is really
invalid given you cannot elide providing libgcc.
[Bug target/86677] popcount builtin detection is breaking some kernel build
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86677
--- Comment #11 from Martin Liška ---
(In reply to Wilco from comment #10)
> (In reply to Martin Liška from comment #9)
> > Taking look at
> > ../drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c file:
> >
> > The __builtin_popcount is generated from:
> >
> > static int brcmf_setup_wiphybands(struct brcmf_cfg80211_info *cfg)
> > {
> > ...
> > } else {
> > for (nchain = 0; rxchain; nchain++)
> >rxchain = rxchain & (rxchain - 1);
> > }
> > ...
> > }
> >
> > It started from r262486. I would that in this situation it's maybe
> > unexpected that a popcount call will be eventually generated. So I'm for an
> > option what will disable the pattern recognition.
>
> I'd say this is exactly a case where using a faster popcount instruction
> helps - if available of course.
Yes, but as seen in PR87528, using a libgcc can slow down a SPEC benchmark. But
it's related to generic tuning.
>
> Is there any reason -fno-builtin-popcount can't also block automatic
> generation of popcount?
Do we really have such option?
[Bug target/86677] popcount builtin detection is breaking some kernel build
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86677
--- Comment #10 from Wilco ---
(In reply to Martin Liška from comment #9)
> Taking look at
> ../drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c file:
>
> The __builtin_popcount is generated from:
>
> static int brcmf_setup_wiphybands(struct brcmf_cfg80211_info *cfg)
> {
> ...
> } else {
> for (nchain = 0; rxchain; nchain++)
>rxchain = rxchain & (rxchain - 1);
> }
> ...
> }
>
> It started from r262486. I would that in this situation it's maybe
> unexpected that a popcount call will be eventually generated. So I'm for an
> option what will disable the pattern recognition.
I'd say this is exactly a case where using a faster popcount instruction helps
- if available of course.
Is there any reason -fno-builtin-popcount can't also block automatic generation
of popcount?
[Bug target/86677] popcount builtin detection is breaking some kernel build
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86677
Martin Liška changed:
What|Removed |Added
CC||marxin at gcc dot gnu.org
--- Comment #9 from Martin Liška ---
Taking look at ../drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
file:
The __builtin_popcount is generated from:
static int brcmf_setup_wiphybands(struct brcmf_cfg80211_info *cfg)
{
...
} else {
for (nchain = 0; rxchain; nchain++)
rxchain = rxchain & (rxchain - 1);
}
...
}
It started from r262486. I would that in this situation it's maybe unexpected
that a popcount call will be eventually generated. So I'm for an option what
will disable the pattern recognition.
[Bug target/86677] popcount builtin detection is breaking some kernel build
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86677 Will Deacon changed: What|Removed |Added CC||will.deacon at arm dot com --- Comment #8 from Will Deacon --- I replied to the ticket raised on the kernel.org bugzilla about this change: https://bugzilla.kernel.org/show_bug.cgi?id=200671#c1 I've also duplicated my response below in case you'd rather respond here. --->8 Whilst providing an implementation of __popcountsi2 will fix the build, won't this end up with worse code generation compared to a compiler which doesn't do this idiom recognition? If I understand this correctly, an in-line integer popcount implementation in the code can be spotted by the compiler and replaced by a branch to an out-of-line integer popcount implementation. Please can we have an option to disable this idiom recognition? It really doesn't seem to make sense in an environment where the SIMD registers aren't readily accessible.
[Bug target/86677] popcount builtin detection is breaking some kernel build
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86677 --- Comment #7 from rguenther at suse dot de --- On October 12, 2018 6:57:44 PM GMT+02:00, "wilco at gcc dot gnu.org" wrote: >https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86677 > >Wilco changed: > > What|Removed |Added > > CC||wilco at gcc dot gnu.org > >--- Comment #6 from Wilco --- >(In reply to rguent...@suse.de from comment #5) >> On Wed, 10 Oct 2018, ktkachov at gcc dot gnu.org wrote: >> >> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86677 >> > >> > ktkachov at gcc dot gnu.org changed: >> > >> >What|Removed |Added >> > > >> > CC||ktkachov at gcc >dot gnu.org >> > >> > --- Comment #3 from ktkachov at gcc dot gnu.org --- >> > GCC does disable some pattern recognition with >> > -fno-tree-loop-distribute-patterns, which is implied by >-ffreestanding (used by >> > the kernel). Wouldn't it be consistent to disable this pattern >recognition in >> > that setup as well? popcount is not such a fundamental helper >function like >> > e.g. DImode shifts, after all >> >> I am not against adding a new switch for this (not sure if we really >> should overload -fno-tree-loop-distribute-patterns with this since >> this will disable popcount recognition at anything below -O3). > >Would it not make sense to check that the popcount optab is implemented >and >enabled before using it? Calling a library function that does a similar >loop >will be slower than keeping the original loop. Sure. There's a thread on gcc patches for this.
[Bug target/86677] popcount builtin detection is breaking some kernel build
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86677 Wilco changed: What|Removed |Added CC||wilco at gcc dot gnu.org --- Comment #6 from Wilco --- (In reply to rguent...@suse.de from comment #5) > On Wed, 10 Oct 2018, ktkachov at gcc dot gnu.org wrote: > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86677 > > > > ktkachov at gcc dot gnu.org changed: > > > >What|Removed |Added > > > > CC||ktkachov at gcc dot gnu.org > > > > --- Comment #3 from ktkachov at gcc dot gnu.org --- > > GCC does disable some pattern recognition with > > -fno-tree-loop-distribute-patterns, which is implied by -ffreestanding > > (used by > > the kernel). Wouldn't it be consistent to disable this pattern recognition > > in > > that setup as well? popcount is not such a fundamental helper function like > > e.g. DImode shifts, after all > > I am not against adding a new switch for this (not sure if we really > should overload -fno-tree-loop-distribute-patterns with this since > this will disable popcount recognition at anything below -O3). Would it not make sense to check that the popcount optab is implemented and enabled before using it? Calling a library function that does a similar loop will be slower than keeping the original loop.
[Bug target/86677] popcount builtin detection is breaking some kernel build
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86677 --- Comment #5 from rguenther at suse dot de --- On Wed, 10 Oct 2018, ktkachov at gcc dot gnu.org wrote: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86677 > > ktkachov at gcc dot gnu.org changed: > >What|Removed |Added > > CC||ktkachov at gcc dot gnu.org > > --- Comment #3 from ktkachov at gcc dot gnu.org --- > GCC does disable some pattern recognition with > -fno-tree-loop-distribute-patterns, which is implied by -ffreestanding (used > by > the kernel). Wouldn't it be consistent to disable this pattern recognition in > that setup as well? popcount is not such a fundamental helper function like > e.g. DImode shifts, after all I am not against adding a new switch for this (not sure if we really should overload -fno-tree-loop-distribute-patterns with this since this will disable popcount recognition at anything below -O3).
[Bug target/86677] popcount builtin detection is breaking some kernel build
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86677 Alexander Monakov changed: What|Removed |Added CC||amonakov at gcc dot gnu.org --- Comment #4 from Alexander Monakov --- PR 87528 is also suggesting to avoid introducing popcount when it's going to be lowered to a libgcc call instead of a native instruction.
[Bug target/86677] popcount builtin detection is breaking some kernel build
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86677 ktkachov at gcc dot gnu.org changed: What|Removed |Added CC||ktkachov at gcc dot gnu.org --- Comment #3 from ktkachov at gcc dot gnu.org --- GCC does disable some pattern recognition with -fno-tree-loop-distribute-patterns, which is implied by -ffreestanding (used by the kernel). Wouldn't it be consistent to disable this pattern recognition in that setup as well? popcount is not such a fundamental helper function like e.g. DImode shifts, after all
[Bug target/86677] popcount builtin detection is breaking some kernel build
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86677
--- Comment #2 from kugan at gcc dot gnu.org ---
(In reply to Richard Biener from comment #1)
> The kernel simply has to provide __popcount{s,d}i2 like it provides other
> libgcc functions if it chooses to not link against libgcc.
Yes, I created this bug just so that I can point it to the kernel people. I
will raise it with the kernel people internally and see what I can do. Thanks.
[Bug target/86677] popcount builtin detection is breaking some kernel build
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86677
Richard Biener changed:
What|Removed |Added
Status|UNCONFIRMED |RESOLVED
CC||rguenth at gcc dot gnu.org
Resolution|--- |INVALID
--- Comment #1 from Richard Biener ---
The kernel simply has to provide __popcount{s,d}i2 like it provides other
libgcc functions if it chooses to not link against libgcc.
