On Fri, Oct 30, 2020 at 11:09 AM Stefan Schulze Frielinghaus via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > On Wed, Oct 28, 2020 at 11:34:53AM -0600, Jeff Law wrote: > > > > On 10/28/20 11:29 AM, Stefan Schulze Frielinghaus wrote: > > > On Wed, Oct 28, 2020 at 08:39:41AM -0600, Jeff Law wrote: > > >> On 10/28/20 3:38 AM, Stefan Schulze Frielinghaus via Gcc-patches wrote: > > >>> On Mon, Oct 05, 2020 at 02:02:57PM +0200, Stefan Schulze Frielinghaus > > >>> via Gcc-patches wrote: > > >>>> On Tue, Sep 22, 2020 at 02:59:30PM +0200, Andreas Krebbel wrote: > > >>>>> On 15.09.20 17:02, Stefan Schulze Frielinghaus wrote: > > >>>>>> Over the last couple of months quite a few warnings about > > >>>>>> uninitialized > > >>>>>> variables were raised while building GCC. A reason why these > > >>>>>> warnings > > >>>>>> show up on S/390 only is due to the aggressive inlining settings > > >>>>>> here. > > >>>>>> Some of these warnings (2c832ffedf0, b776bdca932, 2786c0221b6, > > >>>>>> 1657178f59b) could be fixed or in case of a false positive silenced > > >>>>>> by > > >>>>>> initializing the corresponding variable. Since the latter reoccurs > > >>>>>> and > > >>>>>> while bootstrapping such warnings are turned into errors > > >>>>>> bootstrapping > > >>>>>> fails on S/390 consistently. Therefore, for the moment do not turn > > >>>>>> those warnings into errors. > > >>>>>> > > >>>>>> config/ChangeLog: > > >>>>>> > > >>>>>> * warnings.m4: Do not turn maybe-uninitialized warnings into > > >>>>>> errors > > >>>>>> on S/390. > > >>>>>> > > >>>>>> fixincludes/ChangeLog: > > >>>>>> > > >>>>>> * configure: Regenerate. > > >>>>>> > > >>>>>> gcc/ChangeLog: > > >>>>>> > > >>>>>> * configure: Regenerate. > > >>>>>> > > >>>>>> libcc1/ChangeLog: > > >>>>>> > > >>>>>> * configure: Regenerate. > > >>>>>> > > >>>>>> libcpp/ChangeLog: > > >>>>>> > > >>>>>> * configure: Regenerate. > > >>>>>> > > >>>>>> libdecnumber/ChangeLog: > > >>>>>> > > >>>>>> * configure: Regenerate. > > >>>>> That change looks good to me. Could a global reviewer please comment! > > >>>> Ping > > >>> Ping > > >> I think this would be a huge mistake to install. > > > The root cause why those false positives show up on S/390 only seems to > > > be of more aggressive inlining w.r.t. other architectures. Because of > > > bigger caches and a rather huge function call overhead we greatly > > > benefit from those inlining parameters. Thus: > > > > > > 1) Reverting those parameters would have a negative performance impact. > > > > > > 2) Fixing the maybe-uninitialized warnings analysis itself seems not to > > > happen in the near future (assuming that it is fixable at all). > > > > > > 3) Silencing the warning by initialising the variable itself also seems > > > to be undesired and feels like a fight against windmills ;-) > > > > > > 4) Not lifting maybe-uninitialized warnings to errors on S/390 only. > > > > > > Option (4) has the least intrusive effect to me. At least then it is > > > not necessary to bootstrap with --disable-werror and we would still > > > treat all other warnings as errors. All maybe-uninitialized warnings > > > which are triggered in common code with non-aggressive inlining are > > > still caught by other architectures. Therefore, I'm wondering why this > > > should be a huge mistake? What would you propose instead? > > > > I'm aware of all that. What I think it all argues is that y'all need to > > address the issues because of how you've changed the tuning on the s390 > > port. Simply disabling things like you've suggested is, IMHO, horribly > > wrong. > > > > > > Improve the analysis, dummy initializers, pragmas all seem viable. But > > again, it feels like it's something the s390 maintainers will have to > > take the lead on because of how you've retuned the port. > > Fixing the analysis is of course the best option. However, this sounds > like a non-trivial task to me and I'm missing a lot of context here, > i.e., I'm not sure what the initial goals were and if it is possible to > meet those with the requirements which are necessary to solve those > false positives (currently having PR96564 in mind where it was mentioned > that alias info is not enough but also flow-based info is required; does > this imply that we would have to reschedule the analysis at later time > which was not desired in the first place etc.). > > In the past I tried to come up with some dummy initializers which were > tough to get accepted (which I can understand up to some degree). For > example, this one is still open (I would be happy if you could have a > look at it and accept/reject): > https://gcc.gnu.org/pipermail/gcc-patches/2020-June/547063.html > > Then there is at least one unreported case (similar to PR96564) where we > are not talking about a variable of scalar type but of an aggregate > where only one struct member must be initialized in order to silence the > warning. Not sure whether a patch would be accepted where I initialize > the whole structure or just a single member. > > Thus I'm still willing to come up with dummy initializer patches, > though, I'm not sure whether they are really accepted by the community > or not. > > > And note that this isn't just an issue with uninitialized warnings, the > > changes in inlining heuristics can impact all the middle end warnings. > > Just curious, is this a hypothetical problem or did we have other > problems with those inlining parameters in the past? If there are > further concrete problems with those parameters I would be really > interested to look into those. > > Furthermore, I'm still wondering, if those parameters are that > controversial whether we should document that. It is tempting to take > the documented ranges literally (although admitted we took almost a > limit value for param_inline_min_speedup ;-)). Maybe only a certain > subrange is meant for production? Anyhow, I did a quick test for > param_max_inline_insns_auto which reveals that for values greater than > 18 (default is 15) warnings are emitted. > > I will make a couple of benchmarks in the following days in order to > find a parameter set where no false positive is thrown. What I fear > most is that we get outperformed by other compilers due to less inlining > just because we lifted false positive warnings to errors which feels > really bad to me.
It's not that more / different inlining inherently exposes _more_ false positives in the middle-end warnings. They simply expose others and the GCC codebase is cleansed (by those who change inliner heuristics / tunings) from those by either fixing the analysis or modifying the code (like putting in initializers). The s390 backend is in the exceptional position to change _many_ of the tunables from their default that affect which false positives occur. Besides inliner limits there's also the loop unrolling limits (though those trigger at -O3 only). Whether thats a good or bad tradeoff is up to the port maintainers but I usually urge port maintainers to _not_ change defaults of parameters affecting (relatively) early IL because it makes a testing done on major platforms not be transferable, in terms of coverage, to yours. I also don't believe the telltale that the high inliner limits are necessary to get comparable performance. Note that these were re-tuned only quite some time ago and they are not tuned relative to the current default but tuned to absolute values and thus do not vary when the defaults are re-tuned (which they are for each release). Richard. > Cheers, > Stefan
