[Bug ada/48835] Porting GNAT to GNU/Linux/m68k

["rsandifo at gcc dot gnu.org" <gcc-bugzilla at gcc dot gnu dot org>]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48835

--- Comment #37 from rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> 2011-10-16 13:20:01 UTC ---
(In reply to comment #33)
> (In reply to comment #32)
> > (In reply to comment #31)
> > >         * expmed.c
> > >         (store_bit_field_1): Use the new interfaces.
> > > 
> > > I'll continue trying to minimize the changeset.
> > 
> > Of the three translation paths in store_bit_field_1 that were updated in that
> > revision, vec_set, movstrict, and insv, only the insv path update matters for
> > GNAT/m68k.
> 
> Progress.  The minimal fragment of r171341 that allows r171340 to bootstrap
> GNAT/m68k is the following:
> 
> --- gcc-4.7-r171340/gcc/expmed.c.~1~    2011-03-04 11:31:33.000000000 +0100
> +++ gcc-4.7-r171340/gcc/expmed.c        2011-10-11 09:31:31.000000000 +0200
> @@ -656,7 +656,8 @@ store_bit_field_1 (rtx str_rtx, unsigned
>             && (bitsize + bitpos > GET_MODE_BITSIZE (op_mode)))
>        && insn_data[CODE_FOR_insv].operand[1].predicate (GEN_INT (bitsize),
>                                                         VOIDmode)
> -      && check_predicate_volatile_ok (CODE_FOR_insv, 0, op0, VOIDmode))
> +      && check_predicate_volatile_ok (CODE_FOR_insv, 0, op0,
> +                                    
> insn_data[CODE_FOR_insv].operand[0].mode))
>      {
>        int xbitpos = bitpos;
>        rtx value1;
> 
> That is, when checking insv opnd 0 use the mode from insn_data[] not VOIDmode.
> 
> (The code looks different in r171341 due new APIs and moving the operand
> checking to a later point.  The essential _functional_ difference wrt opnd 0 is
> however just the different mode value used in the check.)
> 
> I'm currently trying to bootstrap GNAT/m68k with gcc-4.6.1 and the above patch.
> 
> Adding Richard Sandiford to CC: list.  Richard, do you have any idea why the
> above mode change might have unbroken m68k?

Sorry for the slow reply.  The SImode/nonimmediate_operand combination
in m68k's insv pattern looks a little odd:

(define_expand "insv"
  [(set (zero_extract:SI (match_operand:SI 0 "nonimmediate_operand" "")
             (match_operand:SI 1 "const_int_operand" "")
             (match_operand:SI 2 "const_int_operand" ""))
    (match_operand:SI 3 "register_operand" ""))]
  "TARGET_68020 && TARGET_BITFIELD"
  "")

The generic insv interface can take two styles of operand:
a QImode memory or a word_mode (SImode) register.  The odd
thing is that this m68k pattern requires even the memories
to be SImode (which they never are).

In 4.6 and earlier, we simply ignored the :SI, so QImode
memories were still accepted.  The effect of the backport
is to restrict insv to register operands (because the mode
is now checked), so the patch is equivalent to changing
nonimmediate_operand to register_operand in the define_expand.
But that isn't really what you want.

In other words, I'm afraid it looks like this backport is
hiding a bug elsewhere that appears somehow (and probably
indirectly!) related to the memory insv define_insns.
Looks like you've also found a performance regression in 4.7,
because I assume we no longer allow memory operands there either.