Re: [RFA][Bug target/108892 ][13 regression] Force re-recognition after changing RTL structure of an insn
On 4/5/23 13:02, Segher Boessenkool wrote: Hi again, On Wed, Apr 05, 2023 at 11:43:30AM -0600, Jeff Law wrote: On 4/5/23 11:38, Segher Boessenkool wrote: Right. But it seems to me it has been there all those years? Does the new testcase fail on older branches? Even if not, it seems clear it is wrong on the older branches as well! I bet if I put the special pattern into an old branch, then ran the testcase it'd probably trigger. But if you think it is too dangerous to backport, let's not. It should be crazy safe. Not a tall worried about it being dangerous. More a case of not seeing a lot of value given how narrow the problem is. Please just do the usual then? git cherry-pick -x $some_hash on the release branches, and push it if that works flawlessly? And if it doesn't, *that* is a good reason to skip backporting it, sure :-) Well, the usual for something like this is to wait, at least for me. Jeff
Re: [RFA][Bug target/108892 ][13 regression] Force re-recognition after changing RTL structure of an insn
Hi again, On Wed, Apr 05, 2023 at 11:43:30AM -0600, Jeff Law wrote: > On 4/5/23 11:38, Segher Boessenkool wrote: > >Right. But it seems to me it has been there all those years? Does the > >new testcase fail on older branches? Even if not, it seems clear it is > >wrong on the older branches as well! > I bet if I put the special pattern into an old branch, then ran the > testcase it'd probably trigger. > > >But if you think it is too dangerous to backport, let's not. > It should be crazy safe. Not a tall worried about it being dangerous. > More a case of not seeing a lot of value given how narrow the problem is. Please just do the usual then? git cherry-pick -x $some_hash on the release branches, and push it if that works flawlessly? And if it doesn't, *that* is a good reason to skip backporting it, sure :-) Segher
Re: [RFA][Bug target/108892 ][13 regression] Force re-recognition after changing RTL structure of an insn
On 4/5/23 11:38, Segher Boessenkool wrote: On Wed, Apr 05, 2023 at 09:07:30AM -0600, Jeff Law wrote: On 4/5/23 08:21, Segher Boessenkool wrote: On Wed, Mar 29, 2023 at 07:48:00AM -0600, Jeff Law wrote: So as mentioned in the PR the underlying issue here is combine changes the form of an existing insn, but fails to force re-recognition. As a result other parts of the compiler blow up. [snip] The fix is trivial, reset the INSN_CODE to force re-recognition in the case where try_combine fails. Thanks for the clear explanation! Okay for trunk. Also okay for all backports (after a week or so on trunk). Thanks. I haven't seen this on any of the release branches, so no strong opinions on backporting at this time. It's a pretty narrow bug (no surprise given its been latent for something like 10 years). Right. But it seems to me it has been there all those years? Does the new testcase fail on older branches? Even if not, it seems clear it is wrong on the older branches as well! I bet if I put the special pattern into an old branch, then ran the testcase it'd probably trigger. But if you think it is too dangerous to backport, let's not. It should be crazy safe. Not a tall worried about it being dangerous. More a case of not seeing a lot of value given how narrow the problem is. jeff
Re: [RFA][Bug target/108892 ][13 regression] Force re-recognition after changing RTL structure of an insn
On Wed, Apr 05, 2023 at 09:07:30AM -0600, Jeff Law wrote: > On 4/5/23 08:21, Segher Boessenkool wrote: > >On Wed, Mar 29, 2023 at 07:48:00AM -0600, Jeff Law wrote: > >>So as mentioned in the PR the underlying issue here is combine changes > >>the form of an existing insn, but fails to force re-recognition. As a > >>result other parts of the compiler blow up. > > > >[snip] > > > >>The fix is trivial, reset the INSN_CODE to force re-recognition in the > >>case where try_combine fails. > > > >Thanks for the clear explanation! Okay for trunk. Also okay for all > >backports (after a week or so on trunk). > Thanks. I haven't seen this on any of the release branches, so no > strong opinions on backporting at this time. It's a pretty narrow bug > (no surprise given its been latent for something like 10 years). Right. But it seems to me it has been there all those years? Does the new testcase fail on older branches? Even if not, it seems clear it is wrong on the older branches as well! But if you think it is too dangerous to backport, let's not. Thanks, Segher
Re: [RFA][Bug target/108892 ][13 regression] Force re-recognition after changing RTL structure of an insn
On 4/5/23 08:21, Segher Boessenkool wrote: Hi! On Wed, Mar 29, 2023 at 07:48:00AM -0600, Jeff Law wrote: So as mentioned in the PR the underlying issue here is combine changes the form of an existing insn, but fails to force re-recognition. As a result other parts of the compiler blow up. [snip] The fix is trivial, reset the INSN_CODE to force re-recognition in the case where try_combine fails. Thanks for the clear explanation! Okay for trunk. Also okay for all backports (after a week or so on trunk). Thanks. I haven't seen this on any of the release branches, so no strong opinions on backporting at this time. It's a pretty narrow bug (no surprise given its been latent for something like 10 years). * combine.cc (combine_instructions): Force re-recognition when potentially changing the underlying RTL structure of an insn. When returning the original, might be clearer? Yea. I'll update the ChangeLog entry. Thanks, Jeff
Re: [RFA][Bug target/108892 ][13 regression] Force re-recognition after changing RTL structure of an insn
Hi! On Wed, Mar 29, 2023 at 07:48:00AM -0600, Jeff Law wrote: > So as mentioned in the PR the underlying issue here is combine changes > the form of an existing insn, but fails to force re-recognition. As a > result other parts of the compiler blow up. [snip] > The fix is trivial, reset the INSN_CODE to force re-recognition in the > case where try_combine fails. Thanks for the clear explanation! Okay for trunk. Also okay for all backports (after a week or so on trunk). > * combine.cc (combine_instructions): Force re-recognition when > potentially changing the underlying RTL structure of an insn. When returning the original, might be clearer? Thanks, Segher
