Georg-Johann Lay <a...@gjlay.de> writes:
> 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...
It should be enough in principle. It's just a case of whether you want
to fix reload, hack around it, or take the plunge and switch to LRA.
Having a (subreg (mem)) is probably key here. If it had been
(subreg (reg:HI X)) for some pseudo X then init_subregs_of_mode should
have realised that 31 isn't a valid choice for X.
I think the reload fix would be to honour simplifiable_subregs when
reloading the (subreg (mem)).
> 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.
FWIW, the current init_subregs_of_mode/simplifiable_subregs behaviour
is fairly recent (2014) and CANNOT_CHANGE_MODE_CLASS had been used in
the past to avoid errors like this. Using it that way was always a
hack though.
An alternative would be to add a new macro to control this block in
general_operand:
#ifdef INSN_SCHEDULING
/* On machines that have insn scheduling, we want all memory
reference to be explicit, so outlaw paradoxical SUBREGs.
However, we must allow them after reload so that they can
get cleaned up by cleanup_subreg_operands. */
if (!reload_completed && MEM_P (sub)
&& GET_MODE_SIZE (mode) > GET_MODE_SIZE (GET_MODE (sub)))
return 0;
#endif
The default would still be INSN_SCHEDULING, but AVR could override it
to 1 and reject the same subregs.
That would still be a hack, but at least it would be taking things in
a good direction. Having different rules depending on whether targets
define a scheduler is just a horrible wart that no-one's ever had chance
to fix. If using the code above works well on AVR then that'd be a big
step towards making the code unconditional.
It'd definitely be worth checking how it affects code quality though.
(Although the same goes for the current patch, since C_C_M_C is a pretty
big hammer.)
Thanks,
Richard
