[PATCH] HIGH part of symbol ref is invalid for constant pool

[PATCH] HIGH part of symbol ref is invalid for constant pool

[Jiufu Guo]

The high part of the symbol address is invalid for the constant pool. In
function rs6000_cannot_force_const_mem, we already return true for
"HIGH with UNSPEC" rtx. During debug GCC, I found that
rs6000_cannot_force_const_mem is called for some other HIGH code rtx
expressions which also indicate the high part of a symbol_ref.
For example:
(high:DI (const:DI (plus:DI (symbol_ref:DI ("xx") (const_int 12 [0xc])))))
(high:DI (symbol_ref:DI ("var_1")..)))

In the below case, this kind of rtx could occur in the middle of optimizations
pass but was not dumped to a file. So, no test case is attached to this
patch.

extern const unsigned int __decPOWERS[10];
void
decSetCoeff (int *residue, const unsigned int *up)
{
 unsigned int half = (unsigned int) __decPOWERS1[3] >> 1;

 if (*up >= half)
  *residue = 7;

 return;
}

This patch updates rs6000_cannot_force_const_mem to return true for
rtx with HIGH code.


Bootstrapped and regtested on ppc64le and ppc64.
Is it ok for trunk?

BR,
Jiufu Guo


gcc/ChangeLog:

	* config/rs6000/rs6000.cc (rs6000_cannot_force_const_mem):
	Return true for HIGH code rtx.

---
 gcc/config/rs6000/rs6000.cc | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
index 3ff16b8ae04..c2b10669627 100644
--- a/gcc/config/rs6000/rs6000.cc
+++ b/gcc/config/rs6000/rs6000.cc
@@ -9707,8 +9707,11 @@ rs6000_init_stack_protect_guard (void)
 static bool
 rs6000_cannot_force_const_mem (machine_mode mode ATTRIBUTE_UNUSED, rtx x)
 {
-  if (GET_CODE (x) == HIGH
-      && GET_CODE (XEXP (x, 0)) == UNSPEC)
+  /* High part of a symbol ref/address can not be put into constant pool. e.g.
+     (high:DI (symbol_ref:DI ("var")..)) or
+     (high:DI (unspec:DI [(symbol_ref/u:DI ("*.LC0")..)
+     (high:DI (const:DI (plus:DI (symbol_ref:DI ("xx")) (const_int 12)))).  */
+  if (GET_CODE (x) == HIGH)
     return true;
 
   /* A TLS symbol in the TOC cannot contain a sum.  */
-- 
2.17.1

Re: [PATCH] HIGH part of symbol ref is invalid for constant pool

[Kewen.Lin]

Hi Jeff,

Thanks for the patch, one question is inlined below.

on 2022/7/4 14:58, Jiufu Guo wrote:
> The high part of the symbol address is invalid for the constant pool. In
> function rs6000_cannot_force_const_mem, we already return true for
> "HIGH with UNSPEC" rtx. During debug GCC, I found that
> rs6000_cannot_force_const_mem is called for some other HIGH code rtx
> expressions which also indicate the high part of a symbol_ref.
> For example:
> (high:DI (const:DI (plus:DI (symbol_ref:DI ("xx") (const_int 12 [0xc])))))
> (high:DI (symbol_ref:DI ("var_1")..)))
> 
> In the below case, this kind of rtx could occur in the middle of optimizations
> pass but was not dumped to a file. So, no test case is attached to this
> patch.
> 

Could you help to expand this more on how it affects some tree-optimization pass?
I guess some tree-opt will expand gimple expression to rtx, evaluate the cost
or similar and make some decision basing on it.  If that is the case, you probably
can construct one test case to show that: without this patch, the evaluated cost
or similar looks off, the optimization decision is sub-optimal;  with this patch,
the optimization result is expected.

BR,
Kewen


> extern const unsigned int __decPOWERS[10];
> void
> decSetCoeff (int *residue, const unsigned int *up)
> {
>  unsigned int half = (unsigned int) __decPOWERS1[3] >> 1;
> 
>  if (*up >= half)
>   *residue = 7;
> 
>  return;
> }
> 
> This patch updates rs6000_cannot_force_const_mem to return true for
> rtx with HIGH code.
> 
> 
> Bootstrapped and regtested on ppc64le and ppc64.
> Is it ok for trunk?
> 
> BR,
> Jiufu Guo
> 
> 
> gcc/ChangeLog:
> 
> 	* config/rs6000/rs6000.cc (rs6000_cannot_force_const_mem):
> 	Return true for HIGH code rtx.
> 
> ---
>  gcc/config/rs6000/rs6000.cc | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index 3ff16b8ae04..c2b10669627 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -9707,8 +9707,11 @@ rs6000_init_stack_protect_guard (void)
>  static bool
>  rs6000_cannot_force_const_mem (machine_mode mode ATTRIBUTE_UNUSED, rtx x)
>  {
> -  if (GET_CODE (x) == HIGH
> -      && GET_CODE (XEXP (x, 0)) == UNSPEC)
> +  /* High part of a symbol ref/address can not be put into constant pool. e.g.
> +     (high:DI (symbol_ref:DI ("var")..)) or
> +     (high:DI (unspec:DI [(symbol_ref/u:DI ("*.LC0")..)
> +     (high:DI (const:DI (plus:DI (symbol_ref:DI ("xx")) (const_int 12)))).  */
> +  if (GET_CODE (x) == HIGH)
>      return true;
> 
>    /* A TLS symbol in the TOC cannot contain a sum.  */

Re: [PATCH] HIGH part of symbol ref is invalid for constant pool

[Jiufu Guo]

"Kewen.Lin" <linkw@linux.ibm.com> writes:

> Hi Jeff,
>
> Thanks for the patch, one question is inlined below.
>
> on 2022/7/4 14:58, Jiufu Guo wrote:
>> The high part of the symbol address is invalid for the constant pool. In
>> function rs6000_cannot_force_const_mem, we already return true for
>> "HIGH with UNSPEC" rtx. During debug GCC, I found that
>> rs6000_cannot_force_const_mem is called for some other HIGH code rtx
>> expressions which also indicate the high part of a symbol_ref.
>> For example:
>> (high:DI (const:DI (plus:DI (symbol_ref:DI ("xx") (const_int 12 [0xc])))))
>> (high:DI (symbol_ref:DI ("var_1")..)))
>> 
>> In the below case, this kind of rtx could occur in the middle of optimizations
>> pass but was not dumped to a file. So, no test case is attached to this
>> patch.
>> 
>
> Could you help to expand this more on how it affects some tree-optimization pass?
> I guess some tree-opt will expand gimple expression to rtx, evaluate the cost
> or similar and make some decision basing on it.  If that is the case, you probably
> can construct one test case to show that: without this patch, the evaluated cost
> or similar looks off, the optimization decision is sub-optimal;  with this patch,
> the optimization result is expected.

Hi Kewen,

Thanks a lot for your comments!

From my investigations, I find some cases for which
rs6000_cannot_force_const_mem is called on "HIGH code" rtx which is not
UNSPEC sub-code.  The code "decSetCoeff" is an example.

In the middle of "combine" pass, function "recog_for_combine" invokes
"force_const_mem", and the invoking could fail.

	  src = force_const_mem (mode, src);
	  if (src)
	    {
	      SUBST (SET_SRC (pat), src);
	      changed = true;
	    }

Here, the rtx "(high:DI (unspec:DI [(symbol_ref" is the argument for
the example code "decSetCoeff".

I also tried to see if other passes(GIMPLE) could hit this kind cases,
but did not find.


BR,
Jiufu(Jeff)

>
> BR,
> Kewen
>
>
>> extern const unsigned int __decPOWERS[10];
>> void
>> decSetCoeff (int *residue, const unsigned int *up)
>> {
>>  unsigned int half = (unsigned int) __decPOWERS1[3] >> 1;
>> 
>>  if (*up >= half)
>>   *residue = 7;
>> 
>>  return;
>> }
>> 
>> This patch updates rs6000_cannot_force_const_mem to return true for
>> rtx with HIGH code.
>> 
>> 
>> Bootstrapped and regtested on ppc64le and ppc64.
>> Is it ok for trunk?
>> 
>> BR,
>> Jiufu Guo
>> 
>> 
>> gcc/ChangeLog:
>> 
>> 	* config/rs6000/rs6000.cc (rs6000_cannot_force_const_mem):
>> 	Return true for HIGH code rtx.
>> 
>> ---
>>  gcc/config/rs6000/rs6000.cc | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>> 
>> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
>> index 3ff16b8ae04..c2b10669627 100644
>> --- a/gcc/config/rs6000/rs6000.cc
>> +++ b/gcc/config/rs6000/rs6000.cc
>> @@ -9707,8 +9707,11 @@ rs6000_init_stack_protect_guard (void)
>>  static bool
>>  rs6000_cannot_force_const_mem (machine_mode mode ATTRIBUTE_UNUSED, rtx x)
>>  {
>> -  if (GET_CODE (x) == HIGH
>> -      && GET_CODE (XEXP (x, 0)) == UNSPEC)
>> +  /* High part of a symbol ref/address can not be put into constant pool. e.g.
>> +     (high:DI (symbol_ref:DI ("var")..)) or
>> +     (high:DI (unspec:DI [(symbol_ref/u:DI ("*.LC0")..)
>> +     (high:DI (const:DI (plus:DI (symbol_ref:DI ("xx")) (const_int 12)))).  */
>> +  if (GET_CODE (x) == HIGH)
>>      return true;
>> 
>>    /* A TLS symbol in the TOC cannot contain a sum.  */