Re: RISC-V: Folding memory for FP + constant case

[Manolis Tsamis <manolis.tsamis@vrull.eu>]

Hi Jeff,

Thanks for all the info!
Then I'll prepare/test a patch that removes this regcprop limitation
and send it out.
I have already tested that this change is enough to make fmo optimize
leela as well.

Manolis

On Fri, Aug 4, 2023 at 7:23 PM Jeff Law <jeffreyalaw@gmail.com> wrote:
>
>
>
> On 8/4/23 03:52, Manolis Tsamis wrote:
> > Hi all,
> >
> > It is true that regcprop currently does not propagate sp and hence
> > leela is not optimized, but from what I see this should be something
> > we can address.
> >
> > The reason that the propagation fails is this check that I have added
> > when I introduced maybe_copy_reg_attrs:
> >
> > else if (REG_POINTER (new_reg) != REG_POINTER (old_reg))
> >    {
> >      /* Only a single instance of STACK_POINTER_RTX must exist and we cannot
> >         modify it. Allow propagation if REG_POINTER for OLD_REG matches and
> >         don't touch ORIGINAL_REGNO and REG_ATTRS. */
> >      return NULL_RTX;
> >    }
> >
> > To be honest I did add this back then just to be on the safe side of
> > whether a mismatch in REG_POINTER after propagation would be an issue
> > (since the original regcprop had caused enough problems).
> No worries.  Obviously not propagating is the safe thing to do and if we
> find notable missed cases we can always refine the existing code like
> we're considering now.
>
> >
> > I see two ways to solve this and make fmo able to optimize leela as well:
> >   1) Remove the REG_POINTER check in regcprop if that is safe. My
> > understanding is that REG_POINTER is used as a hint and there would be
> > no correctness issues.
> REG_POINTER is meant to be conservatively correct.  If REG_POINTER is
> set, then the register must be a pointer to a valid memory location.  If
> REG_POINTER is not set, then it may or may not point to a valid memory
> location.  sp, fp, ap are by definition pointers and thus have
> REG_POINTER set.
>
> With that definition we can safely copy propagate away an sp->gpr copy
> from a REG_POINTER standpoint.
>
>
>
>
>
>
>
>
>
>
> >   2) Mark the corresponding registers with REG_POINTER. I'm not sure
> > where that is supposed to happen.
> >
> > Since the instructions look like this:
> >    (insn 113 11 16 2 (set (reg:DI 15 a5 [226])
> >            (reg/f:DI 2 sp)) 179 {*movdi_64bit}
> >         (nil))
> >
> > I assume that we'd want to mark a5 as REG_POINTER anyway (which is
> > not), and in that case propagation would work.
> I'd strongly recommend against that.  The problem is the destination
> register might have been used in another context as an index register.
> We've fixed a few bugs in this space through the years I suspect there
> may be others lurking.  You can't directly set that up in C code, but
> once you get down to RTL it can happen.
>
>
> > On the other hand if there's no correctness issue w.r.t. REG_POINTER
> > and regcprop then removing the additional check would increase
> > propagation opportunities in general which is also good.
> I think you can safely remove this limitation.
>
> jeff