Re: [Patch,AVR]: FIX ICE in optabs due to bad rotate expander.

[Denis Chertykov <chertykov@gmail.com>]

2011/4/15 Georg-Johann Lay <avr@gjlay.de>:
> Denis Chertykov schrieb:
>> 2011/4/14 Georg-Johann Lay <avr@gjlay.de>:
>>> Denis Chertykov schrieb:
>>>> 2011/4/14 Georg-Johann Lay <avr@gjlay.de>:
>>>>> The "rotl<mode>3" expanders (mode \in {HI,SI,DI}) violates synopsis by
>>>>> using 4 operands instead of 3. This runs in ICE in top of
>>>>> optabs.c:maybe_gen_insn.
>>>>>
>>>>> The right way to do this is to use match_dup, not match_operand. So
>>>>> the fix is obvious.
>>>>>
>>>>> Regenerated avr-gcc and run against avr testsuite:
>>>>>
>>>>> gcc.target/avr/torture/pr41885.c generates these patterns
>>>>>
>>>>> Johann
>>>>>
>>>>> 2011-04-14  Georg-Johann Lay  <avr@gjlay.de>
>>>>>
>>>>>        * config/avr/avr.md ("rotlhi3", "rotlsi3", "rotldi3"): Fix
>>>>>        expanders operand 3 from match_operand to match_dup.
>>>> May be better to use (clobber (match_scratch ...)) here and in
>>>> `*rotw<mode>' and `*rotb<mode>'.
>>> These are splitters that might split before reload, producing strange,
>>> non-matching insns like
>>>  (set (scratch:QI) ...
>>> or crashing already in avr_rotate_bytes becase the split condition reads
>>> "&& (reload_completed || <MODE>mode == DImode)"
>>
>> Generally I'm agree with you, change match_operand to match_dup is easy.
>
> I already submitted the easy patch to avoid the ICE.
>
>> But IMHO the right way is:
>>  - the splitters and avr_rotate_bytes are wrong and must be fixed too.
>>  - operands[3] is a scratch register and right way is to use match_scratch.
>>
>> I can approve the patch.
>>
>> Denis.
>
> Ok, here is the right-way patch.
>
> The expander generates a SCRATCH instead of a pseudo, and in case of a
> pre-reload split the SCRATCH is replaced with a pseudo because it is
> not known if or if not the scratch will actually be needed.
>
> avr_rotate_bytes now skips operations on non-REG 'scratch'.
> Furthermore, I added an assertion that ensures that 'scratch' is
> actually a REG when it is needed.
>
> Besides that, I fixed indentation. I know it is not optimal to fix
> indentation alongside with functionality... did it anyway :-)
>
> Finally, I exposed alternative #3 of the insns to the register
> allocator, because it is not possible to distinguish between
> overlapping or non-overlapping regs, and #3 does not need a scratch.
>
> Ran C-testsuite with no regressions.

Are you encountered any difference in code size ?

Denis.