Re: [PATCH 1/2] Implementation of new RISCV optimizations pass: fold-mem-offsets.

[Jeff Law <jeffreyalaw@gmail.com>]

On 6/12/23 01:32, Manolis Tsamis wrote:

>>
>>> +  for (ref_link = ref_chain; ref_link; ref_link = ref_link->next)
>>> +    {
>>> +      /* Problem getting some definition for this instruction.  */
>>> +      if (ref_link->ref == NULL)
>>> +     return NULL;
>>> +      if (DF_REF_INSN_INFO (ref_link->ref) == NULL)
>>> +     return NULL;
>>> +      if (global_regs[REGNO (reg)]
>>> +       && !set_of (reg, DF_REF_INSN (ref_link->ref)))
>>> +     return NULL;
>>> +    }
>> That last condition feels a bit odd.  It would seem that you wanted an
>> OR boolean rather than AND.
>>
> 
> Most of this function I didn't write by myself, I used existing code
> to get definitions taken from REE's get_defs.
> In the original there's a comment about this line this comment that explains it:
> 
>    As global regs are assumed to be defined at each function call
>    dataflow can report a call_insn as being a definition of REG.
>    But we can't do anything with that in this pass so proceed only
>    if the instruction really sets REG in a way that can be deduced
>    from the RTL structure.
> 
> This function is the only one I copied without changing much (because
> I didn't quite understand it), so I don't know if that condition is
> any useful for f-m-o.
> Also the code duplication here is a bit unfortunate, maybe it would be
> preferred to create a generic version that can be used in both?
Ah.  So I think the code is meant to filter out things that DF will say 
are set vs those which are actually exposed explicitly in the RTL (and 
which REE might be able to modify).  So we're probably good.

Those routines are are pretty close to each other in implementation.  I 
bet we could take everything up to the loop over the ref links and 
factor that into a common function.   Both your function and get_defs 
would be able to use that and then do bit of processing afterwards.


> 
>>
>>> +
>>> +  unsigned int dest_regno = REGNO (dest);
>>> +
>>> +  /* We don't want to fold offsets from instructions that change some
>>> +     particular registers with potentially global side effects.  */
>>> +  if (!GP_REG_P (dest_regno)
>>> +      || dest_regno == STACK_POINTER_REGNUM
>>> +      || (frame_pointer_needed && dest_regno == HARD_FRAME_POINTER_REGNUM)
>>> +      || dest_regno == GP_REGNUM
>>> +      || dest_regno == THREAD_POINTER_REGNUM
>>> +      || dest_regno == RETURN_ADDR_REGNUM)
>>> +    return 0;
>> I'd think most of this would be captured by testing fixed_registers
>> rather than trying to list each register individually.  In fact, if we
>> need to generalize this to work on other targets we almost certainly
>> want a more general test.
>>
> 
> Thanks, I knew there would be some proper way to test this but wasn't
> aware which is the correct one.
> Should this look like below? Or is the GP_REG_P redundant and just
> fixed_regs will do?
If you want to verify it's a general register, then you have to ask if 
the regno is in the GENERAL_REGS class.  Something like:

TEST_HARD_REG_BIT (reg_class_contents[GENERAL_REGS], dest_regno)

GP_REG_P is a risc-v specific macro, so we can't use it here.

So something like
   if (fixed_regs[dest_regno]
       || !TEST_HARD_REG_BIT (reg_class_contents[GENERAL_REGS], dest_regno))



>> The debugging message is a bit misleading.  Yea, we always delete
>> something here, but in one case we end up emitting a copy.
>>
> 
> Indeed. Maybe "Instruction reduced to move: ..."?
Works for me.

> 
>>
>>
>>> +
>>> +       /* Temporarily change the offset in MEM to test whether
>>> +          it results in a valid instruction.  */
>>> +       machine_mode mode = GET_MODE (mem_addr);
>>> +       XEXP (mem, 0) = gen_rtx_PLUS (mode, reg, GEN_INT (offset));
>>> +
>>> +       bool valid_change = recog (PATTERN (insn), insn, 0) >= 0;
>>> +
>>> +       /* Restore the instruction.  */
>>> +       XEXP (mem, 0) = mem_addr;
>> You need to reset the INSN_CODE after restoring the instruction.  That's
>> generally a bad thing to do, but I've seen it done enough (and been
>> guilty myself in the past) that we should just assume some ports are
>> broken in this regard.
>>
> 
> Ok thanks, I didn't knew that one.
It's pretty obscure and I probably would have missed it if I hadn't 
debugged a problem related to this just a few months back.  It shouldn't 
be necessary due to rules about how the movXX patterns are supposed to 
work.  But I've seen it mucked up enough that it's the right thing to do.

Essentially when you call into recog, if the pattern is recognized, then 
a cookie is cached so that we know what pattern was recognized within 
the backend.

As far as generalizing to a target independent pass.  You'll need to 
declare the new pass in tree-pass.h, add it to passes.def, wire up the 
option in common.opt, document it in invoke.texi and turn it on for -O2 
and above.  WRT the interaction with shorten-memrefs, I think that can 
be handled in override-options.  I think we want to have shorten-memrefs 
override.  So if shorten-memrefs is on, then we turn off f-o-m in the RV 
backend.  This should probably be documented in invoke.texi as well.

It sounds like a lot, but I think each of those is a relatively simple 
change.  It'd be best if you could tackle those changes.  I think with 
that done and bootstrap/regression test round we'd be ready to integrate 
your work.

Thanks!

jeff