[RFC] Add a new argument to SELECT_CC_MODE

> simplify_comparison does not change NEW_CODE, OP0, OP1. And we come to
> the code calling SELECT_CC_MODE.
>
>    compare_mode = SELECT_CC_MODE (new_code, op0, op1);
>
> So comparing to X at the beginning of simplify_set, both OP0 and OP1
> have been changed.

Thanks.  As a matter of fact, if you don't call SELECT_CC_MODE when nothing 
has changed, you get code quality regressions for the ARM, apparently because 
of missing changes to CC_NOOVmode.  So my original idea was bogus.

The attached patch is sufficient to fix the regression and doesn't introduce 
new ones for the ARM according to my limited testing.  I think it makes more 
sense because we're preserving the inner compare mode instead of the outer, 
although in the case at hand this is equivalent.  You might want to give it a 
more complete testing though.


	* combine.c (simplify_set): Try harder to find the best CC mode when
	simplifying a COMPARE on the RHS.

On 02/07/2011 06:00 PM, Eric Botcazou wrote:
>> simplify_comparison does not change NEW_CODE, OP0, OP1. And we come to
>> the code calling SELECT_CC_MODE.
>>
>>     compare_mode = SELECT_CC_MODE (new_code, op0, op1);
>>
>> So comparing to X at the beginning of simplify_set, both OP0 and OP1
>> have been changed.
>
> Thanks.  As a matter of fact, if you don't call SELECT_CC_MODE when nothing
> has changed, you get code quality regressions for the ARM, apparently because
> of missing changes to CC_NOOVmode.  So my original idea was bogus.
>
> The attached patch is sufficient to fix the regression and doesn't introduce
> new ones for the ARM according to my limited testing.  I think it makes more
> sense because we're preserving the inner compare mode instead of the outer,
> although in the case at hand this is equivalent.  You might want to give it a
> more complete testing though.
>
>
> 	* combine.c (simplify_set): Try harder to find the best CC mode when
> 	simplifying a COMPARE on the RHS.
>

My testing shows no regressions on arm-none-linux-gnueabi target with 
the default multilib for c,c++,libstc++.

However I thought about this kind of method to fix this issue before. I 
was worried about that the new OP0 and OP1 might need a different mode. 
I thought it might be safer to call SELECT_CC_MODE with a preference. 
For targets like ARM, selecting mode only by the code and operands are 
not enough. For example, we can select CCFPmode or CCFPEmode for GE and 
two SFmode operands. To decide which one is better, we need another 
augument, e.g. BOOL EXCEPTION_P, to say if we want the one which will 
trigger an exception or not. From this point, I choose the method I used 
in my patch. But we still need a better way to find what's for 
EXCEPTION_P when calling SELECT_CC_MODE. Currently I just pass an 
original mode as a roughly preference.

Regards,

> However I thought about this kind of method to fix this issue before. I
> was worried about that the new OP0 and OP1 might need a different mode.

The problem is precisely that OP0 and OP1 aren't really new here, they are the 
unmodified operands of the inner compare.  I think that, because of the 
nested COMPARE, they couldn't have been themselves simplified during this 
combine round, but you indeed never know with the combiner.

Maybe we can explicitly detect the nested COMPARE pattern in order to preserve 
the inner mode if nothing else changed.

> I thought it might be safer to call SELECT_CC_MODE with a preference.

That could indeed work, but then you'd need to pass the mode of the inner 
compare, which is the one to be preserved, and not that of the outer one.

In fact, one could argue that the nested COMPARE is invalid.  It is generated 
a few lines below:

      /* Otherwise, if we didn't previously have a COMPARE in the
	 correct mode, we need one.  */
      if (GET_CODE (src) != COMPARE || GET_MODE (src) != compare_mode)
	{
	  SUBST (SET_SRC (x), gen_rtx_COMPARE (compare_mode, op0, op1));
	  src = SET_SRC (x);
	}
      else if (GET_MODE (op0) == compare_mode && op1 == const0_rtx)
	{
	  SUBST (SET_SRC (x), op0);
	  src = SET_SRC (x);
	}

and you can make it disappear just by swapping the two blocks, which is 
correct I think.  Then you'd generate a no-op move instead, which is nice.
The problem is that this no-op move isn't immediately removed from the insn 
stream and you re-combine it, which triggers the original problem again.

> For targets like ARM, selecting mode only by the code and operands are
> not enough. For example, we can select CCFPmode or CCFPEmode for GE and
> two SFmode operands. To decide which one is better, we need another
> augument, e.g. BOOL EXCEPTION_P, to say if we want the one which will
> trigger an exception or not. From this point, I choose the method I used
> in my patch. But we still need a better way to find what's for
> EXCEPTION_P when calling SELECT_CC_MODE. Currently I just pass an
> original mode as a roughly preference.

Of course the combiner doesn't know anything about exceptions, it can only try 
and preserve what it is passed.

On 02/09/2011 05:03 AM, Jie Zhang wrote:
> However I thought about this kind of method to fix this issue before. I
> was worried about that the new OP0 and OP1 might need a different mode.

Agreed.  For example (compare (reg:SI N) (const_int 1)) may require the 
zero, sign, and overflow flags, while (compare (reg:SI N) (const_int 0)) 
may require only zero and sign.

Paolo

On 02/09/2011 01:31 PM, Paolo Bonzini wrote:
> On 02/09/2011 05:03 AM, Jie Zhang wrote:
>> However I thought about this kind of method to fix this issue before. I
>> was worried about that the new OP0 and OP1 might need a different mode.
>
> Agreed. For example (compare (reg:SI N) (const_int 1)) may require the
> zero, sign, and overflow flags, while (compare (reg:SI N) (const_int 0))
> may require only zero and sign.

Makes no sense, sorry for the noise. :|

Paolo