dwarf2cfi: Improve cfa_reg comparisons [PR103619]

  On Tue, Dec 14, 2021 at 10:32:21AM -0700, Jeff Law wrote:
> I think the attached testcase should trigger on c6x with -mbig-endian -O2 -g

Thanks.  Finally I see what's going on.  c6x doesn't really need the CFA
with span > 1 (and I bet neither does armbe), the only reason why
dwf_cfa_reg is called is that the code in 13 cases just tries to compare
the CFA against dwf_cfa_reg (some_reg).  And that dwf_cfa_reg on some reg
that usually isn't a CFA reg results in targetm.dwarf_register_span hook
call, which on targets like c6x or armeb and others for some registers
creates a PARALLEL with various REGs in it, then the loop with the assertion
and finally operator== which just notes that the reg is different and fails.

This seems compile time memory and time inefficient.

The following so far untested patch instead adds an extra operator== and !=
for comparison of cfa_reg with rtx, which has the most common case where it
is a different register number done early without actually invoking
dwf_cfa_reg.  This means the assertion in dwf_cfa_reg can stay as is (at
least until some big endian target needs to have hard frame pointer or stack
pointer with span > 1 as well).
I've removed a different assertion there because it is redundant - dwf_regno
already has exactly that assertion in it too.

And I've included those 2 tweaks to avoid creating a REG in GC memory when
we can use {stack,hard_frame}_pointer_rtx which is already initialized to
the same REG we need by init_emit_regs.

Ok for trunk if it passes bootstrap/regtest?

2021-12-14  Jakub Jelinek  <jakub@redhat.com>

	PR debug/103619
	* dwarf2cfi.c (dwf_cfa_reg): Remove gcc_assert.
	(operator==, operator!=): New overloaded operators.
	(dwarf2out_frame_debug_adjust_cfa, dwarf2out_frame_debug_cfa_offset,
	dwarf2out_frame_debug_expr): Compare vars with cfa_reg type directly
	with REG rtxes rather than with dwf_cfa_reg results on those REGs.
	(create_cie_data): Use stack_pointer_rtx instead of
	gen_rtx_REG (Pmode, STACK_POINTER_REGNUM).
	(execute_dwarf2_frame): Use hard_frame_pointer_rtx instead of
	gen_rtx_REG (Pmode, HARD_FRAME_POINTER_REGNUM).


	Jakub
  

On 12/14/2021 1:18 PM, Jakub Jelinek wrote:
> On Tue, Dec 14, 2021 at 10:32:21AM -0700, Jeff Law wrote:
>> I think the attached testcase should trigger on c6x with -mbig-endian -O2 -g
> Thanks.  Finally I see what's going on.  c6x doesn't really need the CFA
> with span > 1 (and I bet neither does armbe), the only reason why
> dwf_cfa_reg is called is that the code in 13 cases just tries to compare
> the CFA against dwf_cfa_reg (some_reg).  And that dwf_cfa_reg on some reg
> that usually isn't a CFA reg results in targetm.dwarf_register_span hook
> call, which on targets like c6x or armeb and others for some registers
> creates a PARALLEL with various REGs in it, then the loop with the assertion
> and finally operator== which just notes that the reg is different and fails.
>
> This seems compile time memory and time inefficient.
>
> The following so far untested patch instead adds an extra operator== and !=
> for comparison of cfa_reg with rtx, which has the most common case where it
> is a different register number done early without actually invoking
> dwf_cfa_reg.  This means the assertion in dwf_cfa_reg can stay as is (at
> least until some big endian target needs to have hard frame pointer or stack
> pointer with span > 1 as well).
> I've removed a different assertion there because it is redundant - dwf_regno
> already has exactly that assertion in it too.
>
> And I've included those 2 tweaks to avoid creating a REG in GC memory when
> we can use {stack,hard_frame}_pointer_rtx which is already initialized to
> the same REG we need by init_emit_regs.
>
> Ok for trunk if it passes bootstrap/regtest?
>
> 2021-12-14  Jakub Jelinek  <jakub@redhat.com>
>
> 	PR debug/103619
> 	* dwarf2cfi.c (dwf_cfa_reg): Remove gcc_assert.
> 	(operator==, operator!=): New overloaded operators.
> 	(dwarf2out_frame_debug_adjust_cfa, dwarf2out_frame_debug_cfa_offset,
> 	dwarf2out_frame_debug_expr): Compare vars with cfa_reg type directly
> 	with REG rtxes rather than with dwf_cfa_reg results on those REGs.
> 	(create_cie_data): Use stack_pointer_rtx instead of
> 	gen_rtx_REG (Pmode, STACK_POINTER_REGNUM).
> 	(execute_dwarf2_frame): Use hard_frame_pointer_rtx instead of
> 	gen_rtx_REG (Pmode, HARD_FRAME_POINTER_REGNUM).
So if someone is unfamiliar with the underlying issues here and needs to 
twiddle dwarf2cfi, how are they supposed to know if they should compare 
directly or use dwf_cfa_reg?

I'm not saying the patch is wrong, just wondering if we're setting 
ourselves up for a maintenance problem going forward.

jeff
  

On Tue, Dec 14, 2021 at 03:05:37PM -0700, Jeff Law wrote:
> > 2021-12-14  Jakub Jelinek  <jakub@redhat.com>
> > 
> > 	PR debug/103619
> > 	* dwarf2cfi.c (dwf_cfa_reg): Remove gcc_assert.
> > 	(operator==, operator!=): New overloaded operators.
> > 	(dwarf2out_frame_debug_adjust_cfa, dwarf2out_frame_debug_cfa_offset,
> > 	dwarf2out_frame_debug_expr): Compare vars with cfa_reg type directly
> > 	with REG rtxes rather than with dwf_cfa_reg results on those REGs.
> > 	(create_cie_data): Use stack_pointer_rtx instead of
> > 	gen_rtx_REG (Pmode, STACK_POINTER_REGNUM).
> > 	(execute_dwarf2_frame): Use hard_frame_pointer_rtx instead of
> > 	gen_rtx_REG (Pmode, HARD_FRAME_POINTER_REGNUM).
> So if someone is unfamiliar with the underlying issues here and needs to
> twiddle dwarf2cfi, how are they supposed to know if they should compare
> directly or use dwf_cfa_reg?

Comparison without dwf_cfa_reg should be used whenever possible, because
for registers which are never CFA related that won't call
targetm.dwarf_register_span uselessly.

The only comparisons with dwf_cfa_reg I've kept are the:
            regno = dwf_cfa_reg (XEXP (XEXP (dest, 0), 0));
          
            if (cur_cfa->reg == regno)
              offset -= cur_cfa->offset;
            else if (cur_trace->cfa_store.reg == regno)
              offset -= cur_trace->cfa_store.offset;
            else
              {       
                gcc_assert (cur_trace->cfa_temp.reg == regno);
                offset -= cur_trace->cfa_temp.offset;
              }
and
            struct cfa_reg regno = dwf_cfa_reg (XEXP (dest, 0));
          
            if (cur_cfa->reg == regno)
              offset = -cur_cfa->offset;
            else if (cur_trace->cfa_store.reg == regno)
              offset = -cur_trace->cfa_store.offset;
            else
              {
                gcc_assert (cur_trace->cfa_temp.reg == regno);
                offset = -cur_trace->cfa_temp.offset;
              }
and there are 2 reasons for it:
1) there is an assertion, which guarantees it must compare equal to one of
those 3 cfa related struct cfa_reg structs, so it must be some CFA related
register (so, right now, targetm.dwarf_register_span shouldn't return
non-NULL in those on anything but gcn)
2) it is compared 3 times in a row, so for the GCN case doing
            if (cur_cfa->reg == XEXP (XEXP (dest, 0), 0))
              offset -= cur_cfa->offset;
            else if (cur_trace->cfa_store.reg == XEXP (XEXP (dest, 0), 0))
              offset -= cur_trace->cfa_store.offset;
            else
              {       
                gcc_assert (cur_trace->cfa_temp.reg == XEXP (XEXP (dest, 0), 0));
                offset -= cur_trace->cfa_temp.offset;
              }
could actually create more GC allocated garbage than the way it is written
now.  But doing it that way would work fine.

I think for most of the comparisons even comparing with dwf_cfa_reg would
work but be less compile time/memory efficient (e.g. those assertions that
it is equal to some CFA related cfa_reg or in any spots where only the CFA
related regs may appear in the frame related patterns).

I'm aware just of a single spot where comparison with dwf_cfa_reg doesn't
work (when the assert is in dwf_cfa_reg), that is the spot that was ICEing
on your testcase, where we save arbitrary call saved register:
      if (REG_P (src)
          && REGNO (src) != STACK_POINTER_REGNUM
          && REGNO (src) != HARD_FRAME_POINTER_REGNUM
          && cur_cfa->reg == src)

	Jakub
  

On 12/14/2021 3:27 PM, Jakub Jelinek wrote:
> On Tue, Dec 14, 2021 at 03:05:37PM -0700, Jeff Law wrote:
>>> 2021-12-14  Jakub Jelinek  <jakub@redhat.com>
>>>
>>> 	PR debug/103619
>>> 	* dwarf2cfi.c (dwf_cfa_reg): Remove gcc_assert.
>>> 	(operator==, operator!=): New overloaded operators.
>>> 	(dwarf2out_frame_debug_adjust_cfa, dwarf2out_frame_debug_cfa_offset,
>>> 	dwarf2out_frame_debug_expr): Compare vars with cfa_reg type directly
>>> 	with REG rtxes rather than with dwf_cfa_reg results on those REGs.
>>> 	(create_cie_data): Use stack_pointer_rtx instead of
>>> 	gen_rtx_REG (Pmode, STACK_POINTER_REGNUM).
>>> 	(execute_dwarf2_frame): Use hard_frame_pointer_rtx instead of
>>> 	gen_rtx_REG (Pmode, HARD_FRAME_POINTER_REGNUM).
>> So if someone is unfamiliar with the underlying issues here and needs to
>> twiddle dwarf2cfi, how are they supposed to know if they should compare
>> directly or use dwf_cfa_reg?
> Comparison without dwf_cfa_reg should be used whenever possible, because
> for registers which are never CFA related that won't call
> targetm.dwarf_register_span uselessly.
So it's easy enough to articulate.   Is there anywhere you could put a 
comment to that effect where it's likely to be seen in that file?

OK with that change.

Jeff