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
