Re: [preliminary patch] Fix rtl-optimization/27538.

[Roger Sayle <roger at eyesopen dot com>]

Hi Kazu,

On Sat, 13 May 2006, Kazu Hirata wrote:
> > This should have pretty much the same behaviour as your patch but
> > without the hackish maybe_simplify_shift_const.  Sound reasonable?
> > I agree that the invariants for simplify_shift_const of result_mode
> > vs. GET_MODE (varop) are pretty ugly through out combine.c.
>
> I agree that your solution would work, but I am wondering if we should do
> something about other uses of simplify_shift_const.  All but one
> invocations of simplify_shift_const pass NULL_RTX as the first
> argument, expecting a correct piece of rtx to be built even though
> it is not guaranteed.

That was an initial concern of mine too, and the first draft of a
review reply to your e-mail was to put your fix in simplify_shift_const
instead of expand_compound_operation or a new maybe_simplify_shift_const.
However, not only can gen_lowpart fail, but also some of the callers
of simplify_shift_const rely on the shift being done in the mode of
varop, instead of result_mode.


>> The shift is normally computed in the widest mode we find in VAROP,
>> as long as it isn't a different number of words than RESULT_MODE.
>> Exceptions are ASHIFTRT and ROTATE, which are always done in their
>> original mode.


However, such a change and/or an audit of all of the callers of
simplify_shift_const is probably too intrusive for this part of stage3.
The first priority to should be to correct the regression that we're
aware of, PR27538, safely before 4.2 branches.

For 4.3 and later the long term goal should be to continue the
transition of combine to use simplify-rtx APIs.  In simplify-rtx,
it's always reasonable to return NULL_RTX, which would solve the
problems we're having here.

In the meantime, I believe the invariant should be that all callers
of simplify_shift_const should ensure the result_mode == GET_MODE (varop),
which guarantee's that we're safe.  Most callers already ensure this,
as the result of a shift or rotate has the same mode as its operand.
It's predominantly this problematic abuse in expand_compound_operation
to implement ZERO_EXTEND/SIGN_EXTEND that's not enforcing type/mode
safety in invoking simplify_shift_const.

This should rationalize why the change I suggested was reasonable.
If we wanted to be more aggressive about it, we could enforce this
in simplify_shift_const:


/* Simplify a shift of VAROP in machine mode MODE by COUNT bits.
   CODE says what kind of shift.  If we cannot simplify it,
   return X or, if it is NULL, synthesize the expression with
   simplify_gen_binary.  Otherwise, return a simplified value.*/

static rtx
simplify_shift_const (rtx x, enum rtx_code code, enum machine_mode mode,
                      rtx varop, int count)
{
  rtx tem;

  gcc_assert (GET_MODE (varop) == mode);
  gcc_assert (!x || GET_MODE (x) == mode);

  tem = simplify_shift_const_1 (code, mode, varop, count);
  if (tem)
    return tem;

  if (!x)
    x = simplify_gen_binary (code, mode, varop, GEN_INT (count));
  return x;
}


This would ensure that we never again run into problems trying to
take SUBREGs of volatile variables.  The callers should be audited
by hand, and a few bootstrap and regression tests will confirm that
we haven't missed anything.


So my recommendation would be go with the previous patch (to fix
the regression and is a pre-requisite for the change above) now.
Test the above change, and update the necessary callers to identify
any remaining problems.  Post the resulting patch to gcc-patches
and we can decide if it's safe/reasonable for 4.2, or should wait
until stage1 of 4.3.

Sound reasonable?

Roger
--