This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [patch, avr] Take 2: Fix PR64331: insn output and insn length computation rely on REG_DEAD notes.
- From: Denis Chertykov <chertykov at gmail dot com>
- To: Georg-Johann Lay <avr at gjlay dot de>
- Cc: Steven Bosscher <stevenb dot gcc at gmail dot com>, GCC Patches <gcc-patches at gcc dot gnu dot org>
- Date: Mon, 2 Mar 2015 17:19:21 +0400
- Subject: Re: [patch, avr] Take 2: Fix PR64331: insn output and insn length computation rely on REG_DEAD notes.
- Authentication-results: sourceware.org; auth=none
- References: <54EB06C0 dot 2080503 at gjlay dot de> <54EF7591 dot 3030900 at gjlay dot de> <CABu31nMM1TKLRK8dfsdrrtKpfzK97d2nPOiEZCLLavbAH2jLQg at mail dot gmail dot com> <CADOs=zYw-xFMF18nKL+xOboDLHG36w-ZPGWApajdaEu5wbh+iQ at mail dot gmail dot com> <54F45858 dot 3060605 at gjlay dot de>
2015-03-02 15:32 GMT+03:00 Georg-Johann Lay <avr@gjlay.de>:
> Am 02/28/2015 um 09:02 AM schrieb Denis Chertykov:
>
>> 2015-02-27 1:45 GMT+03:00 Steven Bosscher <stevenb.gcc@gmail.com>:
>>>
>>> On Thu, Feb 26, 2015 at 8:35 PM, Georg-Johann Lay wrote:
>>>>
>>>> Take #2 introduces a new, avr-specific rtl pass whose sole purpose is to
>>>> rectify notes. The pass is scheduled right before cfg does down (right
>>>> before .*free_cfg) so that cfg and hence df machinery is available.
>>>>
>>>> Regression tests look fine and for the test case the patches produce
>>>> correct
>>>> code and correct insn length.
>>>
>>>
>>> Sorry for party-pooping, but it seems to me that the real bug is that
>>> the target is lying to reload.
>>>
>>> IIUC the AVR port selects the insn alternative after register
>>> allocation (after reload). It bases its selection on REG_DEAD notes.
>>> In PR64331 an alternative is used that clobbers r20 that has a
>>> REG_DEAD note, but r20 is not actually dead because hardreg-cprop has
>>> propagated it forward without adjusting the note.
>>>
>>> The "normal" way of things is that the insn alternative is selected in
>>> reload (or in LRA) and that the clobbers are added as necessary. In
>>> PR64331, an alternative for insn r17 would be selected that has a
>>> CLOBBER for r20, prevent hardreg-cprop from propagating r20.
>>>
>>> Selecting insns based on REG-notes is dangerous business. Lying to
>>> reload and to post-RA passes is a mortal sin, the compiler will punish
>>> you. There is no guarantee that nothing will change between your new
>>> pass to recompute notes, and the final pass that emits the insns.
>>>
>>> It's not my port, for sure, but I would look for a real fix instead:
>>> Don't select insns to output based on unreliable information like
>>> REG-notes.
>>
>>
>> Steven rights.
>> We will have an endless fight with this problem.
>
>
> Just consider the patch as prerequisite for further changes atop of it as
> outlined in
>
> https://gcc.gnu.org/ml/gcc-patches/2015-02/msg01745.html
>
> The current patches are operating correctly and do nothing wrong. Complete
> rewriting of reg_unused_after will be much more work and more error prone
> (e.g. unrecognizable insn, optimization flaws).
>
>
>> Better to completely drop `reg_unused_after'. (I know that it used
>> around 40 times in port)
>>
>> What do you think Georg ?
>
>
> I'd prefer to fix it, c.f. link above.
Ok.
Please fix it.
Denis.
PS: As I remember:
I have copyed `reg_unused_after' from sparc port.
It was around 10 years ago.
`reg_unused_after' was removed from sparc a few years ago.
So, only avr and sh ports use `reg_unused_after'.
It's dangerous because GCC core developers don't bothered about two
small ports. (Ports with relatively small user base)
At least we can remove `reg_unused_after' in any time.
(It's relatively easy.)