On Thu, Nov 19, 2015 at 01:38:53PM +0000, Kyrill Tkachov wrote: > >That is troublesome. Could you look deeper? > > Yes.
Thanks. > So the bad case is when we're in subst and returning a CLOBBER of zero > and 'from' is (reg/v:SI 80 [ x ]) and 'to' is (zero_extend:SI (reg:HI 0 x0 > [ x ])). > The call to subst comes from try_combine at line 3403: > > if (added_sets_1) > { > rtx t = i1pat; > if (i0_feeds_i1_n) > t = subst (t, i0dest, i0src_copy ? i0src_copy : i0src, 0, 0, 0); > > XVECEXP (newpat, 0, --total_sets) = t; > } > > It uses t after calling subst on it without checking that it didn't return > a clobber. > If I change that snippet to check for CLOBBER: > if (added_sets_1) > { > rtx t = i1pat; > if (i0_feeds_i1_n) > t = subst (t, i0dest, i0src_copy ? i0src_copy : i0src, 0, 0, 0); > > if (GET_CODE (t) == CLOBBER) > { > undo_all (); > return 0; > } > XVECEXP (newpat, 0, --total_sets) = t; > } > > The testcase gets fixed. > But shouldn't the XVECEXP (newpat, 0, --total_sets) = t; line create an > uncrecognizable rtx > that would then get rejected by combine or something? Yes. recog_for_combine_1 checks for a PARALLEL with such a CLOBBER right at the start; and of course having the clobber elsewhere will just not match. > If we don't check for clobber there and perform the "XVECEXP = ..." > the resulting newpat looks like: > (parallel [ > (set (reg:CC 66 cc) > (compare:CC (const_int 0 [0]) > (const_int 0 [0]))) > (nil) > (clobber:DI (const_int 0 [0])) > ]) > > ah, so the clobber is put in a parallel with another insn and is thus > accepted by recog? No, recog_for_combine should refuse it because of that third arm. The second arm (the nil) looks very wrong, where is that coming from? That isn't valid RTL. > So, should we check 't' after subst for clobbers as above? Or should this > be fixed in > some other place? There is a bug somewhere, so that will need fixing. Workarounds are last resort, and even then we really need to know what is going on. > Thanks. In light of the above I think this patch happens to avoid > the issue highlighted above but we should fix the above code separately? Yes, if your patch creates better code we want it (and fix the regression), but you exposed a separate bug as well :-) Segher