diff mbox series

[v3,2/2] RISC-V: avoid LUI based const mat in alloca epilogue expansion

Message ID 20240520233237.109269-2-vineetg@rivosinc.com
State New
Headers show
Series [v3,1/2] RISC-V: avoid LUI based const mat in prologue/epilogue expansion [PR/105733] | expand

Commit Message

Vineet Gupta May 20, 2024, 11:32 p.m. UTC 
This is testsuite clean however there's a dwarf quirk which I want to
run by the experts. The test that was tripping CI has following
fragment:

	Before patch		|	After Patch
------------------------------------------------------
   li	t0,-4096		|  addi	sp,s0,-2048
   addi	t0,t0,560		|  .cfi_def_cfa 2, 2048      <- #1
   add	sp,s0,t0		|  addi	sp,sp,-1488
   .cfi_def_cfa 2, 3536		|  .cfi_def_cfa_offset 3536  <- #2
   addi	sp,sp,1504		|  addi	sp,sp,1504
   .cfi_def_cfa_offset 2032	|  .cfi_def_cfa_offset 2032  <- #3

The dwarf insn #1 and #3 seem ok, however #2 seems dubious to me.

---

This is continuing on the prev patch in function epilogue expansion.

gcc/ChangeLog:
	* config/riscv/riscv.cc (riscv_expand_epilogue): Handle offset
	being sum of two S12.

Signed-off-by: Vineet Gupta <vineetg@rivosinc.com>
---
 gcc/config/riscv/riscv.cc | 33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

Comments

Jeff Law May 21, 2024, 3:54 a.m. UTC | #1 
On 5/20/24 5:32 PM, Vineet Gupta wrote:
> This is testsuite clean however there's a dwarf quirk which I want to
> run by the experts. The test that was tripping CI has following
> fragment:
> 
> 	Before patch		|	After Patch
> ------------------------------------------------------
>     li	t0,-4096		|  addi	sp,s0,-2048
>     addi	t0,t0,560		|  .cfi_def_cfa 2, 2048      <- #1
>     add	sp,s0,t0		|  addi	sp,sp,-1488
>     .cfi_def_cfa 2, 3536		|  .cfi_def_cfa_offset 3536  <- #2
>     addi	sp,sp,1504		|  addi	sp,sp,1504
>     .cfi_def_cfa_offset 2032	|  .cfi_def_cfa_offset 2032  <- #3
> 
> The dwarf insn #1 and #3 seem ok, however #2 seems dubious to me.
What about it seems dubious?  We need a CFA adjustment on each insn that 
modifies the stack pointer so that we can unwind at any arbitrary point.

The first adjustment says the prior frame is at sp + 2048.  Then it's at 
sp + 3536.  Then after the final insn the prior frame is at sp+2032.

Jeff
Vineet Gupta May 21, 2024, 3:15 p.m. UTC | #2 
On 5/20/24 20:54, Jeff Law wrote:
> On 5/20/24 5:32 PM, Vineet Gupta wrote:
>> This is testsuite clean however there's a dwarf quirk which I want to
>> run by the experts. The test that was tripping CI has following
>> fragment:
>>
>> 	Before patch		|	After Patch
>> ------------------------------------------------------
>>     li	t0,-4096		|  addi	sp,s0,-2048
>>     addi	t0,t0,560		|  .cfi_def_cfa 2, 2048      <- #1
>>     add	sp,s0,t0		|  addi	sp,sp,-1488
>>     .cfi_def_cfa 2, 3536		|  .cfi_def_cfa_offset 3536  <- #2
>>     addi	sp,sp,1504		|  addi	sp,sp,1504
>>     .cfi_def_cfa_offset 2032	|  .cfi_def_cfa_offset 2032  <- #3
>>
>> The dwarf insn #1 and #3 seem ok, however #2 seems dubious to me.
> What about it seems dubious?  

My discomfort at claiming I understand dwarf, despite debugging/fixing
the ARC Linux port's in kernel dwarf unwinder :-)

> We need a CFA adjustment on each insn that 
> modifies the stack pointer so that we can unwind at any arbitrary point.

Of course.

> The first adjustment says the prior frame is at sp + 2048.  Then it's at 
> sp + 3536.  Then after the final insn the prior frame is at sp+2032.

Yeah I got confused with second one since once it gets anchored to SP
from S0, but you are right it is farther from base CFA now.

-Vineet
Jeff Law May 21, 2024, 5:38 p.m. UTC | #3 
On 5/20/24 5:32 PM, Vineet Gupta wrote:
> This is testsuite clean however there's a dwarf quirk which I want to
> run by the experts. The test that was tripping CI has following
> fragment:
> 
> 	Before patch		|	After Patch
> ------------------------------------------------------
>     li	t0,-4096		|  addi	sp,s0,-2048
>     addi	t0,t0,560		|  .cfi_def_cfa 2, 2048      <- #1
>     add	sp,s0,t0		|  addi	sp,sp,-1488
>     .cfi_def_cfa 2, 3536		|  .cfi_def_cfa_offset 3536  <- #2
>     addi	sp,sp,1504		|  addi	sp,sp,1504
>     .cfi_def_cfa_offset 2032	|  .cfi_def_cfa_offset 2032  <- #3
> 
> The dwarf insn #1 and #3 seem ok, however #2 seems dubious to me.
> 
> ---
> 
> This is continuing on the prev patch in function epilogue expansion.
> 
> gcc/ChangeLog:
> 	* config/riscv/riscv.cc (riscv_expand_epilogue): Handle offset
> 	being sum of two S12.
OK.
jeff
diff mbox series

Patch

diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index 2ecbcf1d0af8..85df5b7ab498 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -8111,7 +8111,10 @@  riscv_expand_epilogue (int style)
       need_barrier_p = false;
 
       poly_int64 adjust_offset = -frame->hard_frame_pointer_offset;
+      rtx dwarf_adj = gen_int_mode (adjust_offset, Pmode);
       rtx adjust = NULL_RTX;
+      bool sum_of_two_s12 = false;
+      HOST_WIDE_INT one, two;
 
       if (!adjust_offset.is_constant ())
 	{
@@ -8123,14 +8126,23 @@  riscv_expand_epilogue (int style)
 	}
       else
 	{
-	  if (!SMALL_OPERAND (adjust_offset.to_constant ()))
+	  HOST_WIDE_INT adj_off_value = adjust_offset.to_constant ();
+	  if (SMALL_OPERAND (adj_off_value))
+	    {
+	      adjust = GEN_INT (adj_off_value);
+	    }
+	  else if (SUM_OF_TWO_S12_ALGN (adj_off_value))
+	    {
+	      riscv_split_sum_of_two_s12 (adj_off_value, &one, &two);
+	      dwarf_adj = adjust = GEN_INT (one);
+	      sum_of_two_s12 = true;
+	    }
+	  else
 	    {
 	      riscv_emit_move (RISCV_PROLOGUE_TEMP (Pmode),
-			       GEN_INT (adjust_offset.to_constant ()));
+			       GEN_INT (adj_off_value));
 	      adjust = RISCV_PROLOGUE_TEMP (Pmode);
 	    }
-	  else
-	    adjust = GEN_INT (adjust_offset.to_constant ());
 	}
 
       insn = emit_insn (
@@ -8138,14 +8150,21 @@  riscv_expand_epilogue (int style)
 			      adjust));
 
       rtx dwarf = NULL_RTX;
-      rtx cfa_adjust_value = gen_rtx_PLUS (
-			       Pmode, hard_frame_pointer_rtx,
-			       gen_int_mode (-frame->hard_frame_pointer_offset, Pmode));
+      rtx cfa_adjust_value = gen_rtx_PLUS (Pmode, hard_frame_pointer_rtx,
+					   dwarf_adj);
       rtx cfa_adjust_rtx = gen_rtx_SET (stack_pointer_rtx, cfa_adjust_value);
       dwarf = alloc_reg_note (REG_CFA_ADJUST_CFA, cfa_adjust_rtx, dwarf);
+
       RTX_FRAME_RELATED_P (insn) = 1;
 
       REG_NOTES (insn) = dwarf;
+
+      if (sum_of_two_s12)
+	{
+	  insn = emit_insn (gen_add3_insn (stack_pointer_rtx, stack_pointer_rtx,
+			    GEN_INT (two)));
+	  RTX_FRAME_RELATED_P (insn) = 1;
+	}
     }
 
   if (use_restore_libcall || use_multi_pop)