Re: [patch,avr] PR78883: Implement CANNOT_CHANGE_MODE_CLASS.

[Denis Chertykov <chertykov at gmail dot com>]

2017-01-02 19:22 GMT+04:00 Georg-Johann Lay <avr@gjlay.de>:
> On 02.01.2017 15:54, Dominik Vogt wrote:
>>
>> On Mon, Jan 02, 2017 at 03:47:43PM +0100, Georg-Johann Lay wrote:
>>>
>>> This fixes PR78883 which is a problem in reload revealed by a
>>> change to combine.c.  The fix is as proposed by Segher: implement
>>> CANNOT_CHANGE_MODE_CLASS.
>>>
>>> Ok for trunk?
>>>
>>> Johann
>>>
>>>
>>> gcc/
>>>         PR target/78883
>>>         * config/avr/avr.h (CANNOT_CHANGE_MODE_CLASS): New define.
>>>         * config/avr/avr-protos.h (avr_cannot_change_mode_class): New
>>> proto.
>>>         * config/avr/avr.c (avr_cannot_change_mode_class): New function.
>>>
>>> gcc/testsuite/
>>>         PR target/78883
>>>         * gcc.c-torture/compile/pr78883.c: New test.
>>
>>
>>> Index: config/avr/avr-protos.h
>>> ===================================================================
>>> --- config/avr/avr-protos.h     (revision 244001)
>>> +++ config/avr/avr-protos.h     (working copy)
>>> @@ -111,7 +111,7 @@ extern int _reg_unused_after (rtx_insn *
>>>  extern int avr_jump_mode (rtx x, rtx_insn *insn);
>>>  extern int test_hard_reg_class (enum reg_class rclass, rtx x);
>>>  extern int jump_over_one_insn_p (rtx_insn *insn, rtx dest);
>>> -
>>> +extern int avr_cannot_change_mode_class (machine_mode, machine_mode,
>>> enum reg_class);
>>>  extern int avr_hard_regno_mode_ok (int regno, machine_mode mode);
>>>  extern void avr_final_prescan_insn (rtx_insn *insn, rtx *operand,
>>>                                     int num_operands);
>>> Index: config/avr/avr.c
>>> ===================================================================
>>> --- config/avr/avr.c    (revision 244001)
>>> +++ config/avr/avr.c    (working copy)
>>> @@ -11833,6 +11833,21 @@ jump_over_one_insn_p (rtx_insn *insn, rt
>>>  }
>>>
>>>
>>> +/* Worker function for `CANNOT_CHANGE_MODE_CLASS'.  */
>>> +
>>> +int
>>> +avr_cannot_change_mode_class (machine_mode from, machine_mode to,
>>> +                              enum reg_class /* rclass */)
>>> +{
>>> +  /* We cannot access a hard register in a wider mode, for example we
>>> +     must not access (reg:QI 31) as (reg:HI 31).  HARD_REGNO_MODE_OK
>>> +     would avoid such hard regs, but reload would generate it anyway
>>> +     from paradoxical subregs of mem, cf. PR78883.  */
>>> +
>>> +  return GET_MODE_SIZE (to) > GET_MODE_SIZE (from);
>>
>>
>> I understand how this fixes the ICE, but is it really necessary to
>> suppress conversions to a wider mode for lower numbered registers?
>
>
> If there is a better hook, I'll propose an according patch.
>
> My expectation was that HARD_REGNO_MODE_OK would be enough to keep
> reload from putting HI into odd registers (and in particular into R31).
> But this is obviously not the case...
>
> And internals are not very helpful here.  It only mentions modifying
> ordinary subregs of pseudos, but not paradoxical subreg of memory.
>
> What's also astonishing me is that this problem never popped up
> during the last > 15 years of avr back-end.

May be it's a related problem: https://gcc.gnu.org/ml/gcc/2011-02/msg00444.html

>
> Johann
>
>
>
>>> +}
>>> +
>>> +
>>>  /* Worker function for `HARD_REGNO_MODE_OK'.  */
>>>  /* Returns 1 if a value of mode MODE can be stored starting with hard
>>>     register number REGNO.  On the enhanced core, anything larger than
>>> Index: config/avr/avr.h
>>> ===================================================================
>>> --- config/avr/avr.h    (revision 244001)
>>> +++ config/avr/avr.h    (working copy)
>>> @@ -216,6 +216,9 @@ These two properties are reflected by bu
>>>
>>>  #define MODES_TIEABLE_P(MODE1, MODE2) 1
>>>
>>> +#define CANNOT_CHANGE_MODE_CLASS(MFROM, MTO, RCLASS) \
>>> +  avr_cannot_change_mode_class (MFROM, MTO, RCLASS)
>>> +
>>>  enum reg_class {
>>>    NO_REGS,
>>>    R0_REG,                      /* r0 */
>>> Index: testsuite/gcc.c-torture/compile/pr78883.c
>>> ===================================================================
>>> --- testsuite/gcc.c-torture/compile/pr78883.c   (nonexistent)
>>> +++ testsuite/gcc.c-torture/compile/pr78883.c   (working copy)
>>> @@ -0,0 +1,12 @@
>>> +/* { dg-do compile } */
>>> +
>>> +int foo (int *p)
>>> +{
>>> +  int i;
>>> +  for (i = 0; i < 5; i++)
>>> +    {
>>> +      if (p[i] & 1)
>>> +        return i;
>>> +    }
>>> +  return -1;
>>> +}
>>
>>
>> Ciao
>>
>> Dominik ^_^  ^_^
>>
>