On Thu, Oct 5, 2023 at 5:54 PM Jeff Law <jeffreya...@gmail.com> wrote:
>
>
>
> On 10/3/23 05:45, Manolis Tsamis wrote:
> > This is a new RTL pass that tries to optimize memory offset calculations
>
> > +
> > +/* If INSN is a root memory instruction then compute a potentially new
> > offset
> > + for it and test if the resulting instruction is valid. */
> > +static void
> > +do_check_validity (rtx_insn *insn, fold_mem_info *info)
> > +{
> > + rtx mem, reg;
> > + HOST_WIDE_INT cur_offset;
> > + if (!get_fold_mem_root (insn, &mem, ®, &cur_offset))
> > + return;
> > +
> > + HOST_WIDE_INT new_offset = cur_offset + info->added_offset;
> > +
> > + /* Test if it is valid to change MEM's address offset to NEW_OFFSET. */
> > + int icode = INSN_CODE (insn);
> > + rtx mem_addr = XEXP (mem, 0);
> > + machine_mode mode = GET_MODE (mem_addr);
> > + if (new_offset != 0)
> > + XEXP (mem, 0) = gen_rtx_PLUS (mode, reg, gen_int_mode (new_offset,
> > mode));
> > + else
> > + XEXP (mem, 0) = reg;
> > +
> > + bool illegal = insn_invalid_p (insn, false)
> > + || !memory_address_addr_space_p (mode, XEXP (mem, 0),
> > + MEM_ADDR_SPACE (mem));
> > +
> > + /* Restore the instruction. */
> > + XEXP (mem, 0) = mem_addr;
> > + INSN_CODE (insn) = icode;
> > +
> > + if (illegal)
> > + bitmap_ior_into (&cannot_fold_insns, info->fold_insns);
> > + else
> > + bitmap_ior_into (&candidate_fold_insns, info->fold_insns);
> > +}
> > +
> So overnight testing with the latest version of your patch triggered a
> fault on the sh3-linux-gnu target with this code at -O2:
>
> > enum
> > {
> > _ISspace = ((5) < 8 ? ((1 << (5)) << 8) : ((1 << (5)) >> 8)),
> > };
> > extern const unsigned short int **__ctype_b_loc (void)
> > __attribute__ ((__nothrow__ )) __attribute__ ((__const__));
> > void
> > read_alias_file (const char *fname, int fname_len)
> > {
> > char buf[400];
> > char *cp;
> > cp = buf;
> > while (((*__ctype_b_loc ())[(int) (((unsigned char) cp[0]))] &
> > (unsigned short int) _ISspace))
> > ++cp;
> > }
> >
>
>
> The problem is we need to clear the INSN_CODE before we call recog. In
> this specific case we had (mem (plus (reg) (offset)) after f-m-o does
> its job, the offset went to zero so we changed the structure of the RTL
> to (mem (reg)). But we had the old INSN_CODE still in place which
> caused us to reference operands that no longer exist.
>
> A simple INSN_CODE (insn) = -1 before calling_insn_invalid_p is the
> right fix.
>
Thanks for catching that. I had some thoughts that the change that
doesn't emit rtx plus when offset is zero could do something like
this, but I missed that change needed.
Will fix this in the next iteration.
Manolis
> jeff
