Re: [PATCH][RTL ifcvt] PR rtl-optimization/71594: ICE in noce_emit_cmove due to mismatched source modes

[Kyrill Tkachov <kyrylo dot tkachov at foss dot arm dot com>]

Hi Bernd,

On 04/07/16 12:08, Bernd Schmidt wrote:
> On 07/04/2016 12:28 PM, Kyrill Tkachov wrote:
> > Ping.
> > https://gcc.gnu.org/ml/gcc-patches/2016-06/msg01731.html
> >
> > Thanks,
> > Kyrill
> >
> > On 24/06/16 09:32, Kyrill Tkachov wrote:
> > > Hi all,
> > >
> > > In this PR we get an ICE when trying to emit a conditional move
> > > through noce_convert_multiple_sets.
> > > The comment in the patch explains the situation but we get a
> > > two-instruction sequence like:
> > > (insn 20 19 21 3 (set (reg:SI 89 [ _5 ])
> > >         (reg:SI 88 [ _4 ])) wice.c:8 82 {*movsi_internal}
> > >      (nil))
> > > (insn 21 20 25 3 (set (reg:HI 90 [ a_lsm.10 ])
> > >         (subreg:HI (reg:SI 89 [ _5 ]) 0)) wice.c:8 84 {*movhi_internal}
> > >      (nil))
>
> > +
> > +      /* We allow simple lowpart register subreg SET sources in
> > +     bb_ok_for_noce_convert_multiple_sets.  Be careful when processing
> > +     sequences like:
> > +     (set (reg:SI r1) (reg:SI r2))
> > +     (set (reg:HI r3) (subreg:HI (r1)))
> > +     For the second insn new_val or old_val (r1 in this example) will be
> > +     taken from the temporaries and have the wider mode which will not
> > +     match with the mode of the other source of the conditional move, so
> > +     we'll end up trying to emit r4:HI = cond ? (r1:SI) : (r3:HI).
> > +     Wrap the two cmove operands into subregs if appropriate to prevent
> > +     that.  */
> > +      if (GET_MODE (new_val) != GET_MODE (temp))
> > +    {
> > +      machine_mode src_mode = GET_MODE (new_val);
> > +      machine_mode dst_mode = GET_MODE (temp);
> > +      gcc_assert (GET_MODE_SIZE (src_mode) > GET_MODE_SIZE (dst_mode));
> > +      new_val = lowpart_subreg (dst_mode, new_val, src_mode);
>
> The question I have would be what happens if you have the inverse of the sequence you expect, maybe with multi-word regs?
>
> (set (reg:SI 0) (reg:SI x))
> (set (reg:DI y) (reg:DI 0))
>
> That seems like it would fail the assert. Maybe this is something we need to catch in the bb_ok function.

That does seem like it could cause trouble but I couldn't think of how that sequence could appear or what its
semantics would be. Would assigning to the SImode reg 0 in your example not touch the upper bits of the DImode
value?

In any case, bb_ok_for_noce_convert_multiple_sets doesn't keep track of dependencies between the instructions
so I think the best place to handle this case would be in noce_convert_multiple_sets where instead of the assert
in this patch we'd just end_sequence () and return FALSE.
Would that be preferable?

Thanks for the review,
Kyrill


> Bernd