On Thu, Jan 31, 2019 at 8:40 AM Aldy Hernandez <al...@redhat.com> wrote: > > Hi folks. > > The problem here is that Wprintf* uses the evrp_range_analyzer engine, > and doesn't understand that the x_5 != 256 conditional below should make > the RANGE [0, 255], not [0, 256]. > > <bb 3> [local count: 1063004407]: > # RANGE [0, 256] NONZERO 511 > # x_10 = PHI <0(2), x_5(3)> > snprintf (&temp, 4, "%%%02X", x_10); > # RANGE [1, 256] NONZERO 511 > x_5 = x_10 + 1; > if (x_5 != 256) > goto <bb 3>; [98.99%] > else > goto <bb 4>; [1.01%] > > This PR is handled quite easily in the ranger work, but alas there is > nothing for this release. Therefore, I'd like to dedicate as little > time as possible to this PR this time around. > > Various possible solutions are discussed in the PR. I think an easy way > is to lean on loop analysis to refine the PHI result. It turns out the > evrp engine already does this if SCEV data is available. > > As Richard mentioned in the PR, we should avoid code differences > dependent on -W* flags. I have put the loop init code in the pass > itself, but am open to moving it outside, perhaps early in the gate > function ??. > > I also took the liberty of calling loop_optimizer_init() with the > smallest subset of LOOPS_NORMAL which could still fix the problem > (LOOPS_HAVE_PREHEADERS). This avoids unnecessary changes to the CFG. > But really, I'm just guessing here. LOOPS_NORMAL would also work, > albeit creating forwarder blocks. > > Tested on x86-64 Linux. > > What do you think?
As far as I understand fold_return_value is true/false independent of -W* flags (but dependent on -fprintf-return-value or so). This means that your patch avoids the risk of CFG changes dependent on -W* flags. This means you could safely use LOOPS_NORMAL (I'm not sure SCEV is happy with just pre-headers, unfortunately its exact requirements are not documented... - at least I see unconditional dereferences of loop->latch which rules out LOOPS_NO_CFG_MANIPULATION, likewise loop_preheader_edge is mentioned). That said, LOOPS_HAVE_PREHEADERS probably works well enough for both SCEV and niter analysis. Thus, the patch is OK if you adjust the comment a bit, it's perfectly safe to init loops conditional on optimization and running in the gate isn't wanted. I'm also not sure why this should go away with on-demand info -- working in the EVRP DOM walker exactly provides on-demand info (just the DOM style one relies on SCEV / niter analysis for any backedge work). Richard. > > Aldy
