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

[Jeff Law <jeffreyalaw@gmail.com>]

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