Re: [PATCH] Enable shrink wrapping for the RISC-V target.

[Jeff Law <jlaw@ventanamicro.com>]

On 11/2/22 08:12, Manolis Tsamis wrote:
> On Wed, Oct 19, 2022 at 8:16 PM Jeff Law via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>> On 10/18/22 11:35, Palmer Dabbelt wrote:
>>>> I would have expected things to work fine with libcalls, perhaps with
>>>> the exception of the save/restore libcalls.  So that needs deeper
>>>> investigation.
>>> The save/restore libcalls only support saving/restoring a handful of
>>> register configurations (just the saved X registers in the order
>>> they're usually saved in by GCC).  It should be OK for correctness to
>>> over-save registers, but it kind of just un-does the shrink wrapping
>>> so not sure it's worth worrying about at that point.
>>>
>>> There's also some oddness around the save/restore libcall ABI, it's
>>> not the standard function ABI but instead a GCC-internal one.  IIRC it
>>> just uses the alternate link register (ie, t0 instead of ra) but I may
>>> have forgotten something else.
>> I hadn't really dug into it -- I was pretty sure they weren't following
>> the standard ABI based on its name and how I've used similar routines to
>> save space on some targets in the past.  So if we're having problems
>> with shrink-wrapping and libcalls, those two might be worth investigating.
>>
>>
>> But I think the most important takeaway is that shrink wrapping should
>> work with libcalls, there's nothing radically different about libcalls
>> that would make them inherently interact poorly with shrink-wrapping.
>> So that aspect of the shrink-wrapping patch needs deeper investigation.
>>
>> Jeff
> I think I miscommunicated the issue previously because my understanding
> of libcalls wasn't very solid. The guard is against the save/restore libcalls
> specifically; other than that shrink wrapping and libcalls are fine.I think it
> makes sense to leave this check because the prologue/epilogue does
> something similar when using libcall save/restore:
>    frame->mask = 0; /* Temporarily fib that we need not save GPRs. */

Looking more closely, yea, it might have been a miscommunication between 
us WRT libcalls.

  You're testing riscv_use_save_libcall, which is only going to kick in 
when we're using that special function to do register saves/restores.  
While we could, in theory, shrink-wrap that as well, I don't think it's 
worth the additional headache.

>
> Since shrink wrap components are marked by testing frame->mask then
> no registers should be wrapped with the libcall save/restore if I understand
> correctly.

That's my understanding as well after looking at the code more closely.  
Essentially the mask is set to zero for the duration of the call to 
riscv_for_each_saved_regs which will effectively avoid shrink wrapping 
in that case.


>
> Nonetheless, I tested what happens if this guard condition is removed
> and the result is that a RISCV test fails (riscv/pr95252.c). In that case
> a unnecessary save/restore of a register is emitted together with
> inconsistent cfi notes that make dwarf2cfi abort.

ACK.  This is the kind of thing I was referring to above when I said we 
probably could shrink wrap the call to save/restore registers, but it's 
probably not worth the headache right now. I could see someone in the 
embedded space one day trying to tackle this problem, but I don't think 
we need to for this patch to go forward.


>
> To conclude, I believe that this makes the code in the commit fine since
> it only guards against the libcall save/restore case. But I may be still
> missing something about this.

I think you've got it figured out reasonably well.


So the final conclusion is libcalls are resolved to my satisfaction.



Jeff