Re: PR 49145: Another (zero_extend (const_int ...)) in combine

[Eric Botcazou <ebotcazou@adacore.com>]

> I've included the subreg handling as well as the zero_extend handling,
> even though make_compound_operation already handles that case correctly.
> However, I've cowardly not removed the known_cond subreg handling:
>
>   else if (code == SUBREG)
>     {
>       enum machine_mode inner_mode = GET_MODE (SUBREG_REG (x));
>       rtx new_rtx, r = known_cond (SUBREG_REG (x), cond, reg, val);
>
>       if (SUBREG_REG (x) != r)
> 	{
> 	  /* We must simplify subreg here, before we lose track of the
> 	     original inner_mode.  */
> 	  new_rtx = simplify_subreg (GET_MODE (x), r,
> 				 inner_mode, SUBREG_BYTE (x));
> 	  if (new_rtx)
> 	    return new_rtx;
> 	  else
> 	    SUBST (SUBREG_REG (x), r);
> 	}
>
>       return x;
>     }
>
> The new function would also handle the case described in the comment.
> However, I was afraid that we might rely on simplify_subreg being
> used for all simplified operands here, while also relying on it only
> being used for constants in subst.  (This comes from a general
> fear that combine is special in the way that it handles compound
> operations.  Calling the generic simplification routines too often
> could cause us to turn combine's preferred representation into
> the representation normally used elsewhere.)

Frankly, I'm not convinced that the benefits are worth the potential hassle in 
this case.  As you mentioned, the 3 functions already have their own handling 
for SUBREGs, slightly different from each other, so factoring it into a common 
form isn't trivial; now, if you do it and don't defer to the new function for 
the entire handling after that, you don't really simplify the code.

So we're essentially (see below) left with the ZERO_EXTEND case and I'd add the 
handful of missing lines to make_compound_operation and be done with it.

> The known_cond code has the comment:
>
>   /* We don't have to handle SIGN_EXTEND here, because even in the
>      case of replacing something with a modeless CONST_INT, a
>      CONST_INT is already (supposed to be) a valid sign extension for
>      its narrower mode, which implies it's already properly
>      sign-extended for the wider mode.  Now, for ZERO_EXTEND, the
>      story is different.  */
>
> While that is true, I don't see any point in going out of our way
> _not_ to handle sign_extend in the same way.  Or indeed all unary
> operators.  Testing UNARY_P seems justified on the basis that
> simplify_unary_operation requires the mode of the inner operand,
> and that losing that mode without giving simplify_unary_operation
> a chance could therefore lead to wrong results.  I'd rather not
> see us hard-code the knowledge of which operators actually care.

SUBREG and ZERO_EXTEND of CONST_INTs are treated somewhat specially in the 
entire file, see for example do_SUBST.  This isn't the case for other unary 
operators, presumably because this isn't really necessary here.  So I'm not 
convinced that such a generalization is really a good thing in this case.

-- 
Eric Botcazou