On 11/02/2013 10:47 AM, Richard Sandiford wrote:
Kenneth Zadeck <zadeck@naturalbridge.com> writes:
On 11/02/2013 07:06 AM, Richard Sandiford wrote:
The first part of this is a simple type mismatch -- get_max_loop_iterations
returns a widest_int aka max_wide_int -- and I'd have installed it as
obvious. The second part isn't as obvious though. The old code stored
the maximum iterations as:
if (!get_max_loop_iterations (loop, &iter)
|| !iter.fits_shwi ())
iterations_max = const0_rtx;
else
iterations_max = GEN_INT (iter.to_shwi ());
and the new code uses:
if (!get_max_loop_iterations (loop, &iter)
|| !wi::fits_shwi_p (iter))
iterations_max = const0_rtx;
else
iterations_max = immed_wide_int_const (iter, mode);
which includes an extra canonicalisation. I agree it would be good to do
that in principle, but I'm not sure it copes correctly with the case where
the loop iterates 1 << GET_MODE_PRECISION (mode) times. Plus I think the
real fix would be to avoid the host dependence altogether, i.e. get rid
of the fits_shwi_p too.
As it stands, this breaks bfin-elf's pattern, which has:
/* Due to limitations in the hardware (an initial loop count of 0
does not loop 2^32 times) we must avoid to generate a hardware
loops when we cannot rule out this case. */
if (!flag_unsafe_loop_optimizations
&& (unsigned HOST_WIDE_INT) INTVAL (operands[2]) >= 0xFFFFFFFF)
FAIL;
With the sign-extending conversion, this now triggers more often than
it was supposed to.
Since the old "GEN_INT (iter.to_shwi ());" works verbatim in wide-int too,
and since we still use that form in the doloop_begin code, I think we should
just back the immed_wide_int_const out.
Tested on powerpc64-linux-gnu and x86_64-linux-gnu. It fixes some
unwanted testsuite changes in bfin-elf. OK to install?
I dislike this a lot. I think that this is a badly written pattern
in the bfin port if it depends on the host size. As machines get
bigger, (they may not be getting faster but they are still getting
bigger) having traps like this at the portable level will bite us.
But this isn't at the portable level, it's backend-specific code.
It knows that it's dealing with a 32-bit counter.
yes, but the patch is dumbing down the portable part of the compiler.
in truth this code should really be iterations_max =
immed_wide_int_const (iter, mode) with no tests at all.
But like I say, that won't cope with loops that iterate
1 << GET_MODE_PRECISION (mode) times, even if that fits
in a signed HWI. (That case could happen if the hardware
supports it and if the counter starts out as 0.)
The mainline code is self-consistent in that the number of
iterations is passed as a positive signed HWI, using 0 if the
value isn't known or is greater than HOST_WIDE_INT_MAX.
In the current interface, the CONST_INT doesn't really have a mode.
It's just used as a convenient way of passing a HWI to an expander.
I'm not saying that it's the ideal interface. But if we want to change it,
let's do that separately, and test it on targets that have doloops.
Until then, the wide-int branch can provide the current interface
just as easily as mainline.
we do not have a predicate at the rtl level like we do at the tree level
where we can ask if the value can be represented in a mode without
loosing any information. But the proper thing to do here is to check
to see if iter fits in the mode, if it does, then generate the
immed_wide_int_const and if not generate the 0. In this way it does
the right thing for loops that iterate 1<<GET_MODE_PRECISION (mode),
i.e. it will produce a 0. And that will be correct even if the mode is
SI or TI.