On Wed, Feb 12, 2025 at 11:06 AM H.J. Lu <hjl.to...@gmail.com> wrote:
>
> On Wed, Feb 12, 2025 at 5:28 PM Uros Bizjak <ubiz...@gmail.com> wrote:
> >
> > On Wed, Feb 12, 2025 at 6:25 AM H.J. Lu <hjl.to...@gmail.com> wrote:
> > >
> > > Don't assume that stack slots can only be accessed by stack or frame
> > > registers. We first find all registers defined by stack or frame
> > > registers. Then check memory accesses by such registers, including
> > > stack and frame registers.
> > >
> > > gcc/
> > >
> > > PR target/109780
> > > PR target/109093
> > > * config/i386/i386.cc (ix86_update_stack_alignment): New.
> > > (ix86_find_all_reg_use): Likewise.
> > > (ix86_find_max_used_stack_alignment): Also check memory accesses
> > > from registers defined by stack or frame registers.
> > >
> > > gcc/testsuite/
> > >
> > > PR target/109780
> > > PR target/109093
> > > * g++.target/i386/pr109780-1.C: New test.
> > > * gcc.target/i386/pr109093-1.c: Likewise.
> > > * gcc.target/i386/pr109780-1.c: Likewise.
> > > * gcc.target/i386/pr109780-2.c: Likewise.
> >
> > > +/* Find all registers defined with REG. */
> > > +
> > > +static void
> > > +ix86_find_all_reg_use (HARD_REG_SET &stack_slot_access,
> > > + unsigned int reg, auto_bitmap &worklist)
> > > +{
> > > + for (df_ref ref = DF_REG_USE_CHAIN (reg);
> > > + ref != NULL;
> > > + ref = DF_REF_NEXT_REG (ref))
> > > + {
> > > + if (DF_REF_IS_ARTIFICIAL (ref))
> > > + continue;
> > > +
> > > + rtx_insn *insn = DF_REF_INSN (ref);
> > > + if (!NONDEBUG_INSN_P (insn))
> > > + continue;
> > > +
> > > + rtx set = single_set (insn);
> > > + if (!set)
> > > + continue;
> > > +
> >
> > Isn't the above condition a bit too limiting? We can have insn with
> > multiple sets in the chain.
> >
> > The issue at hand is the correctness issue (the program will segfault
> > if registers are not tracked correctly), not some missing
> > optimization. I'd suggest to stay on the safe side and also process
> > PARALLELs. Something similar to e.g. store_data_bypass_p from
> > recog.cc:
> >
> > --cut here--
> > rtx set = single_set (insn);
> > if (set)
> > ix86_find_all_reg_use_1(...);
> >
> > rtx pat = PATTERN (insn);
> > if (GET_CODE (pat) != PARALLEL)
> > return false;
> >
> > for (int i = 0; i < XVECLEN (pat, 0); i++)
> > {
> > rtx exp = XVECEXP (pat, 0, i);
> >
> > if (GET_CODE (exp) == CLOBBER || GET_CODE (exp) == USE)
> > continue;
> >
> > gcc_assert (GET_CODE (exp) == SET);
> >
> > ix86_find_all_reg_use_1(...);
> > }
> > --cut here--
> >
> > The above will make ix86_find_all_reg_use significantly more robust.
> >
> > Uros.
>
> Like this?
Yes.
Thanks,
Uros.
>
> /* Helper function for ix86_find_all_reg_use. */
>
> static void
> ix86_find_all_reg_use_1 (rtx set, HARD_REG_SET &stack_slot_access,
> auto_bitmap &worklist)
> {
> rtx src = SET_SRC (set);
> if (MEM_P (src))
> return;
>
> rtx dest = SET_DEST (set);
> if (!REG_P (dest))
> return;
>
> if (TEST_HARD_REG_BIT (stack_slot_access, REGNO (dest)))
> return;
>
> /* Add this register to stack_slot_access. */
> add_to_hard_reg_set (&stack_slot_access, Pmode, REGNO (dest));
> bitmap_set_bit (worklist, REGNO (dest));
> }
>
> /* Find all registers defined with REG. */
>
> static void
> ix86_find_all_reg_use (HARD_REG_SET &stack_slot_access,
> unsigned int reg, auto_bitmap &worklist)
> {
> for (df_ref ref = DF_REG_USE_CHAIN (reg);
> ref != NULL;
> ref = DF_REF_NEXT_REG (ref))
> {
> if (DF_REF_IS_ARTIFICIAL (ref))
> continue;
>
> rtx_insn *insn = DF_REF_INSN (ref);
> if (!NONDEBUG_INSN_P (insn))
> continue;
>
> rtx set = single_set (insn);
> if (set)
> ix86_find_all_reg_use_1 (set, stack_slot_access, worklist);
>
> rtx pat = PATTERN (insn);
> if (GET_CODE (pat) != PARALLEL)
> continue;
>
> for (int i = 0; i < XVECLEN (pat, 0); i++)
> {
> rtx exp = XVECEXP (pat, 0, i);
>
> if (GET_CODE (exp) == CLOBBER || GET_CODE (exp) == USE)
> continue;
>
> gcc_assert (GET_CODE (exp) == SET);
>
> ix86_find_all_reg_use_1 (exp, stack_slot_access, worklist);
> }
> }
> }
>
>
> --
> H.J.
