On 19/11/15 15:00, Kyrill Tkachov wrote:
On 19/11/15 14:41, Segher Boessenkool wrote:
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.
Well, it came from a bit earlier before the subst call (around line 3390):
/* If the actions of the earlier insns must be kept
in addition to substituting them into the latest one,
we must make a new PARALLEL for the latest insn
to hold additional the SETs. */
<snip>
rtx old = newpat;
total_sets = 1 + extra_sets;
newpat = gen_rtx_PARALLEL (VOIDmode, rtvec_alloc (total_sets));
XVECEXP (newpat, 0, 0) = old;
extra_sets is 2, so we have a parallel with 3 slots so after the subst call
we put the clobber into --total_sets, that is slot 2:
XVECEXP (newpat, 0, --total_sets) = t;
A bit further below we fill slot 1 with another rtx, so all 3 parts of the
PARALLEL
are filled.
I'll look further into why recog_for_combine doesn't kill the whole thing.
Hmmm, so the answer to that is a bit further down the validate_replacement:
path.
It's the code after the big comment:
/* See if this is a PARALLEL of two SETs where one SET's destination is
a register that is unused and this isn't marked as an instruction that
might trap in an EH region. In that case, we just need the other SET.
We prefer this over the PARALLEL.
This can occur when simplifying a divmod insn. We *must* test for this
case here because the code below that splits two independent SETs doesn't
handle this case correctly when it updates the register status.
It's pointless doing this if we originally had two sets, one from
i3, and one from i2. Combining then splitting the parallel results
in the original i2 again plus an invalid insn (which we delete).
The net effect is only to move instructions around, which makes
debug info less accurate. */
The code extracts all the valid sets inside the PARALLEL and calls
recog_for_combine on them
individually, ignoring the clobber. Should we tell it to reject any. So
recog_for_combine doesn't
complain because it never sees the clobbers and all the other checks in
try_combine are gated
on "insn_code < 0" so they never apply.
Where should we add the extra checks for CLOBBER? Or do you reckon we should
add another call to
recog_for_combine somewhere there?
Thanks,
Kyrill
Kyrill
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
