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

Reply via email to