Hi Segher, Thanks for the review.
on 2021/3/19 下午8:36, Segher Boessenkool wrote: > Hi! > > On Fri, Mar 19, 2021 at 10:46:54AM +0800, Kewen.Lin wrote: >> As Segher and Mike pointed out, the define_insn_and_split should avoid >> to use empty split condition if the condition for define_insn isn't empty, >> otherwise it can sometimes leads to unexpected consequence. >> >> This patch is to fix some places like this. >> >> Bootstrapped/regtested on powerpc64le-linux-gnu P9 and >> powerpc64-linux-gnu P8. >> >> Since it's in very low risk and can avoid some unexpected errors, >> is it ok for trunk? or has to be for GCC12? > > I am curious if the splitters ever triggered where they should not have? > Do you have any suggestion to catch this? I thought the regression testing probably can show something different but it didn't unfortunately. >> diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md >> index 592ac90aa44..6ab71979566 100644 >> --- a/gcc/config/rs6000/rs6000.md >> +++ b/gcc/config/rs6000/rs6000.md >> @@ -4281,7 +4281,7 @@ > > If you add > > [diff "md"] > xfuncname = "^\\(define.*$" > > to your .git/config, the patch will show what insn this is in: > Thanks for the tips! >> @@ -4281,7 +4281,7 @@ (define_insn_and_split "*rotldi3_insert_sf" > > > The patch is okay for trunk. Thank you! > > You might want to make this error easier to detect? Maybe make > define_insn_and_split raise an error if the split condition is empty but > the insn condition is not? > Good idea! Is there any possible reason to write this kind of define_insn_and_split? If no (we should forbid it), I think we can add the check and raise an error if hits in gensupport.c. I'll send out a RFC once GCC12 starts. BR, Kewen
