Message ID | 20200807113342.2522717-1-ibuclaw@gdcproject.org |
---|---|
State | New |
Headers | show |
Series | PR target/96347: non-delegitimized UNSPEC UNSPEC_TP (19) found in variable location | expand |
Ping. Though I wonder if there's any point adding a check at all over just swapping the order that mem_loc_descriptor and tls_mem_loc_descriptor are called in. Iain. On 07/08/2020 13:33, Iain Buclaw wrote: > Hi, > > On x86, a memory reference reference to a TLS address can be broken out > and stored in a register, for instance: > > movq %fs:8+testYearsBC@tpoff, %rdx > > Subsequently becomes: > > pushq %rbp > leaq 8+testYearsBC@tpoff, %rbp > // later... > movq %fs:0(%rbp), %rdx > > When it comes to representing this in Dwarf, mem_loc_descriptor is left > with the following RTL, which ix86_delegitimize_tls_address is unable to > handle as the symbol_ref has been swapped out with the temporary. > > (plus:DI (unspec:DI [ > (const_int 0 [0]) > ] UNSPEC_TP) > (reg/f:DI 6 bp [90])) > > The UNSPEC_TP expression is checked, ix86_const_not_ok_for_debug_p > returns false as it only lets through UNSPEC_GOTOFF, and finally > const_ok_for_output is either not smart enough or does not have the > information needed to recognize that UNSPEC_TP is a TLS UNSPEC that > should be ignored. This results in informational warnings being fired > under -fchecking. > > The entrypoint that triggers this warning to occur is that MEM codes are > first lowered with mem_loc_descriptor, with tls_mem_loc_descriptor only > being used if that failed. This patch switches that around so that TLS > memory expressions first call tls_mem_loc_descriptor, and only use > mem_loc_descriptor if that fails (which may result in UNSPEC warnings). > > Bootstrapped and regression tested on x86_64-linux-gnu, I do also have a > sparc64-linux-gnu build at hand, but have not yet got the results to > check for a before/after comparison. > > OK for mainline? > > Regards > Iain > > --- > gcc/ChangeLog: > > PR target/96347 > * dwarf2out.c (is_tls_mem_loc): New. > (mem_loc_descriptor): Call tls_mem_loc_descriptor on TLS memory > expressions, fallback to mem_loc_descriptor if unsuccessful. > (loc_descriptor): Likewise. > > gcc/testsuite/ChangeLog: > > PR target/96347 > * g++.dg/other/pr96347.C: New test. > --- > gcc/dwarf2out.c | 30 ++++++++++---- > gcc/testsuite/g++.dg/other/pr96347.C | 61 ++++++++++++++++++++++++++++ > 2 files changed, 84 insertions(+), 7 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/other/pr96347.C > > diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c > index 9deca031fc2..093ceb23c7a 100644 > --- a/gcc/dwarf2out.c > +++ b/gcc/dwarf2out.c > @@ -14435,6 +14435,20 @@ is_based_loc (const_rtx rtl) > && CONST_INT_P (XEXP (rtl, 1))))); > } > > +/* Return true if this MEM expression is for a TLS variable. */ > + > +static bool > +is_tls_mem_loc (rtx mem) > +{ > + tree base; > + > + if (MEM_EXPR (mem) == NULL_TREE || !MEM_OFFSET_KNOWN_P (mem)) > + return false; > + > + base = get_base_address (MEM_EXPR (mem)); > + return (base && VAR_P (base) && DECL_THREAD_LOCAL_P (base)); > +} > + > /* Try to handle TLS MEMs, for which mem_loc_descriptor on XEXP (mem, 0) > failed. */ > > @@ -15671,11 +15685,12 @@ mem_loc_descriptor (rtx rtl, machine_mode mode, > return mem_loc_result; > } > } > - mem_loc_result = mem_loc_descriptor (XEXP (rtl, 0), > - get_address_mode (rtl), mode, > - VAR_INIT_STATUS_INITIALIZED); > - if (mem_loc_result == NULL) > + if (is_tls_mem_loc (rtl)) > mem_loc_result = tls_mem_loc_descriptor (rtl); > + if (mem_loc_result == NULL) > + mem_loc_result = mem_loc_descriptor (XEXP (rtl, 0), > + get_address_mode (rtl), mode, > + VAR_INIT_STATUS_INITIALIZED); > if (mem_loc_result != NULL) > { > if (!is_a <scalar_int_mode> (mode, &int_mode) > @@ -16631,10 +16646,11 @@ loc_descriptor (rtx rtl, machine_mode mode, > break; > > case MEM: > - loc_result = mem_loc_descriptor (XEXP (rtl, 0), get_address_mode (rtl), > - GET_MODE (rtl), initialized); > - if (loc_result == NULL) > + if (is_tls_mem_loc (rtl)) > loc_result = tls_mem_loc_descriptor (rtl); > + if (loc_result == NULL) > + loc_result = mem_loc_descriptor (XEXP (rtl, 0), get_address_mode (rtl), > + GET_MODE (rtl), initialized); > if (loc_result == NULL) > { > rtx new_rtl = avoid_constant_pool_reference (rtl); > diff --git a/gcc/testsuite/g++.dg/other/pr96347.C b/gcc/testsuite/g++.dg/other/pr96347.C > new file mode 100644 > index 00000000000..414d10c73de > --- /dev/null > +++ b/gcc/testsuite/g++.dg/other/pr96347.C > @@ -0,0 +1,61 @@ > +/* { dg-do compile { target c++11 } } */ > +/* { dg-require-effective-target tls } */ > +/* { dg-options "-O2 -g -fchecking" } */ > + > +struct DArray > +{ > + __SIZE_TYPE__ length; > + int *ptr; > +}; > + > +__thread DArray testYearsBC; > + > +struct FilterResult > +{ > + DArray input; > + bool primed; > + > + static FilterResult Dthis (FilterResult &pthis, DArray r) > + { > + pthis.input = r; > + return pthis; > + } > + > + int front (void) > + { > + if (input.length == 0) > + __builtin_abort (); > + return input.ptr[0]; > + } > +}; > + > +FilterResult filter (DArray range) > +{ > + FilterResult retval; > + FilterResult::Dthis (retval, range); > + return retval; > +} > + > +DArray chain (DArray rs) > +{ > + return rs; > +} > + > +struct SysTime > +{ > + static SysTime Dthis (int); > +}; > + > +int main (void) > +{ > + while (1) > + { > + FilterResult val; > + __builtin_memset (&val, 0, sizeof (FilterResult)); > + val = filter (chain (testYearsBC)); > + int year = val.front (); > + (void)year; > + SysTime::Dthis (0); > + } > + return 0; > +} >
Iain Buclaw via Gcc-patches <gcc-patches@gcc.gnu.org> writes: > Hi, > > On x86, a memory reference reference to a TLS address can be broken out > and stored in a register, for instance: > > movq %fs:8+testYearsBC@tpoff, %rdx > > Subsequently becomes: > > pushq %rbp > leaq 8+testYearsBC@tpoff, %rbp > // later... > movq %fs:0(%rbp), %rdx > > When it comes to representing this in Dwarf, mem_loc_descriptor is left > with the following RTL, which ix86_delegitimize_tls_address is unable to > handle as the symbol_ref has been swapped out with the temporary. > > (plus:DI (unspec:DI [ > (const_int 0 [0]) > ] UNSPEC_TP) > (reg/f:DI 6 bp [90])) > > The UNSPEC_TP expression is checked, ix86_const_not_ok_for_debug_p > returns false as it only lets through UNSPEC_GOTOFF, and finally > const_ok_for_output is either not smart enough or does not have the > information needed to recognize that UNSPEC_TP is a TLS UNSPEC that > should be ignored. This results in informational warnings being fired > under -fchecking. > > The entrypoint that triggers this warning to occur is that MEM codes are > first lowered with mem_loc_descriptor, with tls_mem_loc_descriptor only > being used if that failed. This patch switches that around so that TLS > memory expressions first call tls_mem_loc_descriptor, and only use > mem_loc_descriptor if that fails (which may result in UNSPEC warnings). > > Bootstrapped and regression tested on x86_64-linux-gnu, I do also have a > sparc64-linux-gnu build at hand, but have not yet got the results to > check for a before/after comparison. > > OK for mainline? > > Regards > Iain > > --- > gcc/ChangeLog: > > PR target/96347 > * dwarf2out.c (is_tls_mem_loc): New. > (mem_loc_descriptor): Call tls_mem_loc_descriptor on TLS memory > expressions, fallback to mem_loc_descriptor if unsuccessful. > (loc_descriptor): Likewise. > > gcc/testsuite/ChangeLog: > > PR target/96347 > * g++.dg/other/pr96347.C: New test. > --- > gcc/dwarf2out.c | 30 ++++++++++---- > gcc/testsuite/g++.dg/other/pr96347.C | 61 ++++++++++++++++++++++++++++ > 2 files changed, 84 insertions(+), 7 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/other/pr96347.C > > diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c > index 9deca031fc2..093ceb23c7a 100644 > --- a/gcc/dwarf2out.c > +++ b/gcc/dwarf2out.c > @@ -14435,6 +14435,20 @@ is_based_loc (const_rtx rtl) > && CONST_INT_P (XEXP (rtl, 1))))); > } > > +/* Return true if this MEM expression is for a TLS variable. */ > + > +static bool > +is_tls_mem_loc (rtx mem) > +{ > + tree base; > + > + if (MEM_EXPR (mem) == NULL_TREE || !MEM_OFFSET_KNOWN_P (mem)) > + return false; > + > + base = get_base_address (MEM_EXPR (mem)); > + return (base && VAR_P (base) && DECL_THREAD_LOCAL_P (base)); > +} > + > /* Try to handle TLS MEMs, for which mem_loc_descriptor on XEXP (mem, 0) > failed. */ > > @@ -15671,11 +15685,12 @@ mem_loc_descriptor (rtx rtl, machine_mode mode, > return mem_loc_result; > } > } > - mem_loc_result = mem_loc_descriptor (XEXP (rtl, 0), > - get_address_mode (rtl), mode, > - VAR_INIT_STATUS_INITIALIZED); > - if (mem_loc_result == NULL) > + if (is_tls_mem_loc (rtl)) > mem_loc_result = tls_mem_loc_descriptor (rtl); > + if (mem_loc_result == NULL) Is the is_tls_mem_loc part necessary? It looks like it's just repeating the first part of tls_mem_loc_descriptor, and so we could call that unconditionally instead. I guess this raises the question: if we swapping the order for TLS so that MEM_EXPR trumps the MEM address, why shouldn't we take that approach more generally? I.e. is there any reason to look at the MEM_EXPR only when DECL_THREAD_LOCAL_P is true for the base address, rather than for all symbolic base addresses? Thanks, Richard
Excerpts from Richard Sandiford's message of August 19, 2020 1:22 pm: > Iain Buclaw via Gcc-patches <gcc-patches@gcc.gnu.org> writes: >> Hi, >> >> On x86, a memory reference reference to a TLS address can be broken out >> and stored in a register, for instance: >> >> movq %fs:8+testYearsBC@tpoff, %rdx >> >> Subsequently becomes: >> >> pushq %rbp >> leaq 8+testYearsBC@tpoff, %rbp >> // later... >> movq %fs:0(%rbp), %rdx >> >> When it comes to representing this in Dwarf, mem_loc_descriptor is left >> with the following RTL, which ix86_delegitimize_tls_address is unable to >> handle as the symbol_ref has been swapped out with the temporary. >> >> (plus:DI (unspec:DI [ >> (const_int 0 [0]) >> ] UNSPEC_TP) >> (reg/f:DI 6 bp [90])) >> >> The UNSPEC_TP expression is checked, ix86_const_not_ok_for_debug_p >> returns false as it only lets through UNSPEC_GOTOFF, and finally >> const_ok_for_output is either not smart enough or does not have the >> information needed to recognize that UNSPEC_TP is a TLS UNSPEC that >> should be ignored. This results in informational warnings being fired >> under -fchecking. >> >> The entrypoint that triggers this warning to occur is that MEM codes are >> first lowered with mem_loc_descriptor, with tls_mem_loc_descriptor only >> being used if that failed. This patch switches that around so that TLS >> memory expressions first call tls_mem_loc_descriptor, and only use >> mem_loc_descriptor if that fails (which may result in UNSPEC warnings). >> >> Bootstrapped and regression tested on x86_64-linux-gnu, I do also have a >> sparc64-linux-gnu build at hand, but have not yet got the results to >> check for a before/after comparison. >> >> OK for mainline? >> >> Regards >> Iain >> >> --- >> gcc/ChangeLog: >> >> PR target/96347 >> * dwarf2out.c (is_tls_mem_loc): New. >> (mem_loc_descriptor): Call tls_mem_loc_descriptor on TLS memory >> expressions, fallback to mem_loc_descriptor if unsuccessful. >> (loc_descriptor): Likewise. >> >> gcc/testsuite/ChangeLog: >> >> PR target/96347 >> * g++.dg/other/pr96347.C: New test. >> --- >> gcc/dwarf2out.c | 30 ++++++++++---- >> gcc/testsuite/g++.dg/other/pr96347.C | 61 ++++++++++++++++++++++++++++ >> 2 files changed, 84 insertions(+), 7 deletions(-) >> create mode 100644 gcc/testsuite/g++.dg/other/pr96347.C >> >> diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c >> index 9deca031fc2..093ceb23c7a 100644 >> --- a/gcc/dwarf2out.c >> +++ b/gcc/dwarf2out.c >> @@ -14435,6 +14435,20 @@ is_based_loc (const_rtx rtl) >> && CONST_INT_P (XEXP (rtl, 1))))); >> } >> >> +/* Return true if this MEM expression is for a TLS variable. */ >> + >> +static bool >> +is_tls_mem_loc (rtx mem) >> +{ >> + tree base; >> + >> + if (MEM_EXPR (mem) == NULL_TREE || !MEM_OFFSET_KNOWN_P (mem)) >> + return false; >> + >> + base = get_base_address (MEM_EXPR (mem)); >> + return (base && VAR_P (base) && DECL_THREAD_LOCAL_P (base)); >> +} >> + >> /* Try to handle TLS MEMs, for which mem_loc_descriptor on XEXP (mem, 0) >> failed. */ >> >> @@ -15671,11 +15685,12 @@ mem_loc_descriptor (rtx rtl, machine_mode mode, >> return mem_loc_result; >> } >> } >> - mem_loc_result = mem_loc_descriptor (XEXP (rtl, 0), >> - get_address_mode (rtl), mode, >> - VAR_INIT_STATUS_INITIALIZED); >> - if (mem_loc_result == NULL) >> + if (is_tls_mem_loc (rtl)) >> mem_loc_result = tls_mem_loc_descriptor (rtl); >> + if (mem_loc_result == NULL) > > Is the is_tls_mem_loc part necessary? It looks like it's just > repeating the first part of tls_mem_loc_descriptor, and so we > could call that unconditionally instead. > Not necessary, other than it makes it clear from the caller that rtl is a TLS symbol reference. The comments for both tls_mem_loc_descriptor and mem_loc_descriptor would need to be updated to reflect that the latter is called only if the tls_mem function fails. > I guess this raises the question: if we swapping the order for > TLS so that MEM_EXPR trumps the MEM address, why shouldn't we take > that approach more generally? I.e. is there any reason to look at > the MEM_EXPR only when DECL_THREAD_LOCAL_P is true for the base address, > rather than for all symbolic base addresses? > Someone else might better know. But as I understand, TLS addresses must always be looked up through the thread pointer, even if it is cached to a temporary. For the given test, the generated DWARF doesn't change whether it is either inferred from the MEM_EXPR or the MEM address, should the test be fixed up to not trigger the UNSPEC warning (swap the length and ptr fields). (0x7ffff7615870) DW_OP_const8u address, 0 (0x7ffff76158c0) DW_OP_GNU_push_tls_address 0, 0 (0x7ffff7615960) DW_OP_deref 0, 0 Whereas globals can be dereferenced from anywhere that can hold an address value. Using the test as an example again, removing __thread from the global generates. (0x7ffff7615870) DW_OP_breg6 8, 0 (0x7ffff76158c0) DW_OP_deref 0, 0 Removing the DECL_THREAD_LOCAL_P condition, so that tls_mem_loc_descriptor handles shared globals too instead gives us. (0x7ffff76158c0) DW_OP_addr address, 0 (0x7ffff76159b0) DW_OP_plus_uconst 8, 0 (0x7ffff7615a00) DW_OP_deref 0, 0 What would the benefit of the DW_OP_addr be over DW_OP_breg6? Iain.
diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c index 9deca031fc2..093ceb23c7a 100644 --- a/gcc/dwarf2out.c +++ b/gcc/dwarf2out.c @@ -14435,6 +14435,20 @@ is_based_loc (const_rtx rtl) && CONST_INT_P (XEXP (rtl, 1))))); } +/* Return true if this MEM expression is for a TLS variable. */ + +static bool +is_tls_mem_loc (rtx mem) +{ + tree base; + + if (MEM_EXPR (mem) == NULL_TREE || !MEM_OFFSET_KNOWN_P (mem)) + return false; + + base = get_base_address (MEM_EXPR (mem)); + return (base && VAR_P (base) && DECL_THREAD_LOCAL_P (base)); +} + /* Try to handle TLS MEMs, for which mem_loc_descriptor on XEXP (mem, 0) failed. */ @@ -15671,11 +15685,12 @@ mem_loc_descriptor (rtx rtl, machine_mode mode, return mem_loc_result; } } - mem_loc_result = mem_loc_descriptor (XEXP (rtl, 0), - get_address_mode (rtl), mode, - VAR_INIT_STATUS_INITIALIZED); - if (mem_loc_result == NULL) + if (is_tls_mem_loc (rtl)) mem_loc_result = tls_mem_loc_descriptor (rtl); + if (mem_loc_result == NULL) + mem_loc_result = mem_loc_descriptor (XEXP (rtl, 0), + get_address_mode (rtl), mode, + VAR_INIT_STATUS_INITIALIZED); if (mem_loc_result != NULL) { if (!is_a <scalar_int_mode> (mode, &int_mode) @@ -16631,10 +16646,11 @@ loc_descriptor (rtx rtl, machine_mode mode, break; case MEM: - loc_result = mem_loc_descriptor (XEXP (rtl, 0), get_address_mode (rtl), - GET_MODE (rtl), initialized); - if (loc_result == NULL) + if (is_tls_mem_loc (rtl)) loc_result = tls_mem_loc_descriptor (rtl); + if (loc_result == NULL) + loc_result = mem_loc_descriptor (XEXP (rtl, 0), get_address_mode (rtl), + GET_MODE (rtl), initialized); if (loc_result == NULL) { rtx new_rtl = avoid_constant_pool_reference (rtl); diff --git a/gcc/testsuite/g++.dg/other/pr96347.C b/gcc/testsuite/g++.dg/other/pr96347.C new file mode 100644 index 00000000000..414d10c73de --- /dev/null +++ b/gcc/testsuite/g++.dg/other/pr96347.C @@ -0,0 +1,61 @@ +/* { dg-do compile { target c++11 } } */ +/* { dg-require-effective-target tls } */ +/* { dg-options "-O2 -g -fchecking" } */ + +struct DArray +{ + __SIZE_TYPE__ length; + int *ptr; +}; + +__thread DArray testYearsBC; + +struct FilterResult +{ + DArray input; + bool primed; + + static FilterResult Dthis (FilterResult &pthis, DArray r) + { + pthis.input = r; + return pthis; + } + + int front (void) + { + if (input.length == 0) + __builtin_abort (); + return input.ptr[0]; + } +}; + +FilterResult filter (DArray range) +{ + FilterResult retval; + FilterResult::Dthis (retval, range); + return retval; +} + +DArray chain (DArray rs) +{ + return rs; +} + +struct SysTime +{ + static SysTime Dthis (int); +}; + +int main (void) +{ + while (1) + { + FilterResult val; + __builtin_memset (&val, 0, sizeof (FilterResult)); + val = filter (chain (testYearsBC)); + int year = val.front (); + (void)year; + SysTime::Dthis (0); + } + return 0; +}