On 1/31/19 4:52 AM, Richard Biener wrote:
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
But isn't my code inside of ::execute, which is still gated by
fold_return_value AND all the -W* flags:?
bool
pass_sprintf_length::gate (function *)
{
return ((warn_format_overflow > 0
|| warn_format_trunc > 0
|| flag_printf_return_value)
&& (optimize > 0) == fold_return_value);
}
Thus, I think we need to move the loop initialization somewhere else ??.
(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).
I believe there are backedge smarts in the ranger, at least for the
simple cases. But I'll change the comment about the on-demand code
curing everything, into some reminder to remove the SCEV stuff if/when
we have a more precise range mechanism. I'd hate for this loop
optimization requirement to survive past our need for it.
Aldy
