Re: [PATCH v3] gcc/config/tilegx/tilegx.c (tilegx_function_profiler): Save r10 to stack before call mcount

Re: [PATCH v3] gcc/config/tilegx/tilegx.c (tilegx_function_profiler): Save r10 to stack before call mcount

[Bernd Edlinger]

Hi,

I don't know much about tilegx, but
I think the patch should work as is.

This is because the
Save r10 code is a bundle

  {
  addi sp, sp, -8
  st sp, r10
  }

which stores r10 at [sp] and subtracts 8 from sp.

The restore r10 code is actually two bundles:

  addi sp, sp, 8
  ld r10, sp

This just adds 8 to sp, and loads r10 from there.

I don't know how __mcount is implemented, it must
be some asm code, almost all functions save the
lr at [sp] when invoked, but I don't know if __mcount
does that at all, if it doesn't do that, then the
adjusting of sp might be unnecessary.

The only thing that might be a problem is that
the stack is always adjusted in multiples of 16
on the tilegx platform, see tilegx.h:

#define STACK_BOUNDARY 128

That is counted in bits, and means 16 bytes.
But your patch adjusts the stack only by 8.

Furthermore, I don't see how the stack unwinding will work
with this stack adjustment when no .cfi directives
are emitted, but that is probably not a big problem.

You might see a difference, when single-stepping
over the function entry.

Maybe a test case would be good as well.


Bernd.

Re: [PATCH v3] gcc/config/tilegx/tilegx.c (tilegx_function_profiler): Save r10 to stack before call mcount

[Jeff Law]

On 10/23/2016 12:11 PM, Bernd Edlinger wrote:
> Hi,
>
> I don't know much about tilegx, but
> I think the patch should work as is.
>
> This is because the
> Save r10 code is a bundle
>
>   {
>   addi sp, sp, -8
>   st sp, r10
>   }
>
> which stores r10 at [sp] and subtracts 8 from sp.
>
> The restore r10 code is actually two bundles:
Thanks for pointing that out!  I totally missed the restore was two bundles.


>
>   addi sp, sp, 8
>   ld r10, sp
>
> This just adds 8 to sp, and loads r10 from there.
Right.  And with the restore as two bundles the semantics of the 
save/restore seem consistent/correct.

>
> I don't know how __mcount is implemented, it must
> be some asm code, almost all functions save the
> lr at [sp] when invoked, but I don't know if __mcount
> does that at all, if it doesn't do that, then the
> adjusting of sp might be unnecessary.
>
> The only thing that might be a problem is that
> the stack is always adjusted in multiples of 16
> on the tilegx platform, see tilegx.h:
>
> #define STACK_BOUNDARY 128
>
> That is counted in bits, and means 16 bytes.
> But your patch adjusts the stack only by 8.
Missed that.  Without knowing the tile ports, I can't say with any 
degree of confidence that it's safe to only adjust by 8 bytes. 
Adjusting by 16 seems safer.

>
> Furthermore, I don't see how the stack unwinding will work
> with this stack adjustment when no .cfi directives
> are emitted, but that is probably not a big problem.
I wouldn't be surprised if the tilegx isn't the only port with this 
problem.   I don't think we've ever been good about making sure the 
unwinders are correct for targets where we profile before the prologue 
and which emit the profiling bits directly rather than representing them 
as RTL.

jeff

Re: [PATCH v3] gcc/config/tilegx/tilegx.c (tilegx_function_profiler): Save r10 to stack before call mcount

[Chen Gang]

Firstly, sorry for replying late (During these days, I worked overtime
every workday, and have to reply in weekend).

On 10/24/16 23:27, Jeff Law wrote:
> On 10/23/2016 12:11 PM, Bernd Edlinger wrote:
>> Hi,
>>
>> I don't know much about tilegx, but
>> I think the patch should work as is.
>>
>> This is because the
>> Save r10 code is a bundle
>>
>>   {
>>   addi sp, sp, -8
>>   st sp, r10
>>   }
>>
>> which stores r10 at [sp] and subtracts 8 from sp.
>>
>> The restore r10 code is actually two bundles:
> Thanks for pointing that out!  I totally missed the restore was two bundles.
> 
> 
>>
>>   addi sp, sp, 8
>>   ld r10, sp
>>
>> This just adds 8 to sp, and loads r10 from there.
> Right.  And with the restore as two bundles the semantics of the save/restore seem consistent/correct.
> 

Oh, really. Sorry that I almost forgot my history about this patch.

Originally, I sent patch v1 both with 2 bundles, but when I sent patch
v2, I let "saving r10" within a bundle for optimization (I mentioned
about it in replying patch v1 on 2016-05-31).

>>
>> I don't know how __mcount is implemented, it must
>> be some asm code, almost all functions save the
>> lr at [sp] when invoked, but I don't know if __mcount
>> does that at all, if it doesn't do that, then the
>> adjusting of sp might be unnecessary.
>>
>> The only thing that might be a problem is that
>> the stack is always adjusted in multiples of 16
>> on the tilegx platform, see tilegx.h:
>>
>> #define STACK_BOUNDARY 128
>>
>> That is counted in bits, and means 16 bytes.
>> But your patch adjusts the stack only by 8.
> Missed that.  Without knowing the tile ports, I can't say with any degree of confidence that it's safe to only adjust by 8 bytes. Adjusting by 16 seems safer.
> 

Oh, really! After check all the output code, "addi sp" operation are all
times of 16!! So I guess, I shall addi sp 16, too (send patch v4 for it,
if no any addition reply within a week).

>>
>> Furthermore, I don't see how the stack unwinding will work
>> with this stack adjustment when no .cfi directives
>> are emitted, but that is probably not a big problem.
> I wouldn't be surprised if the tilegx isn't the only port with this problem.   I don't think we've ever been good about making sure the unwinders are correct for targets where we profile before the prologue and which emit the profiling bits directly rather than representing them as RTL.
> 

Excuse me, I have no any idea about it (in fact, in honest, I guess, I
am still not quite familiar with gcc development in details).

At present, what I can know is: after this patch, gcc can pass various
related unwinding test (including nested functions) under qemu tilegx
linux-user (originally, I traced related insns, they should be ok).

:-)

Thanks.
-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.

[PATCH v3] gcc/config/tilegx/tilegx.c (tilegx_function_profiler): Save r10 to stack before call mcount

[chengang]

From: Chen Gang <gang.chen.5i5j@gmail.com>

r10 may also be as parameter stack pointer for the nested function, so
need save it before call mcount.

Also clean up code: use '!' instead of "== 0" for checking
static_chain_decl and compute_total_frame_size.

2016-06-04  Chen Gang  <gang.chen.5i5j@gmail.com>

	gcc/
	PR target/71331
	* config/tilegx/tilegx.c (tilegx_function_profiler): Save r10
	to stack before call mcount.
	(tilegx_can_use_return_insn_p): Clean up code.
---
 gcc/config/tilegx/tilegx.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/gcc/config/tilegx/tilegx.c b/gcc/config/tilegx/tilegx.c
index 06c832c..55161bb 100644
--- a/gcc/config/tilegx/tilegx.c
+++ b/gcc/config/tilegx/tilegx.c
@@ -3880,8 +3880,8 @@ bool
 tilegx_can_use_return_insn_p (void)
 {
   return (reload_completed
-	  && cfun->static_chain_decl == 0
-	  && compute_total_frame_size () == 0
+	  && !cfun->static_chain_decl
+	  && !compute_total_frame_size ()
 	  && tilegx_current_function_is_leaf ()
 	  && !crtl->profile && !df_regs_ever_live_p (TILEGX_LINK_REGNUM));
 }
@@ -5507,6 +5507,15 @@ tilegx_function_profiler (FILE *file, int labelno ATTRIBUTE_UNUSED)
       fprintf (file, "\t}\n");
     }
 
+  if (cfun->static_chain_decl)
+    {
+      fprintf (file,
+	       "\t{\n"
+	       "\taddi\tsp, sp, -8\n"
+	       "\tst\tsp, r10\n"
+	       "\t}\n");
+    }
+
   if (flag_pic)
     {
       fprintf (file,
@@ -5524,6 +5533,13 @@ tilegx_function_profiler (FILE *file, int labelno ATTRIBUTE_UNUSED)
 	       "\t}\n", MCOUNT_NAME);
     }
 
+  if (cfun->static_chain_decl)
+    {
+      fprintf (file,
+	       "\taddi\tsp, sp, 8\n"
+	       "\tld\tr10, sp\n");
+    }
+
   tilegx_in_bundle = false;
 }
 
-- 
1.9.3

Re: [PATCH v3] gcc/config/tilegx/tilegx.c (tilegx_function_profiler): Save r10 to stack before call mcount

[Chen Gang]

Hello Maintainers:

Is this patch OK? Please help check it when you have time (at least for
me, it passes all related test and comparation).

And I shall continue to find and fix another issues about tilegx.

Thanks.

On 6/4/16 21:25, chengang@emindsoft.com.cn wrote:
> From: Chen Gang <gang.chen.5i5j@gmail.com>
> 
> r10 may also be as parameter stack pointer for the nested function, so
> need save it before call mcount.
> 
> Also clean up code: use '!' instead of "== 0" for checking
> static_chain_decl and compute_total_frame_size.
> 
> 2016-06-04  Chen Gang  <gang.chen.5i5j@gmail.com>
> 
> 	gcc/
> 	PR target/71331
> 	* config/tilegx/tilegx.c (tilegx_function_profiler): Save r10
> 	to stack before call mcount.
> 	(tilegx_can_use_return_insn_p): Clean up code.
> ---
>  gcc/config/tilegx/tilegx.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/gcc/config/tilegx/tilegx.c b/gcc/config/tilegx/tilegx.c
> index 06c832c..55161bb 100644
> --- a/gcc/config/tilegx/tilegx.c
> +++ b/gcc/config/tilegx/tilegx.c
> @@ -3880,8 +3880,8 @@ bool
>  tilegx_can_use_return_insn_p (void)
>  {
>    return (reload_completed
> -	  && cfun->static_chain_decl == 0
> -	  && compute_total_frame_size () == 0
> +	  && !cfun->static_chain_decl
> +	  && !compute_total_frame_size ()
>  	  && tilegx_current_function_is_leaf ()
>  	  && !crtl->profile && !df_regs_ever_live_p (TILEGX_LINK_REGNUM));
>  }
> @@ -5507,6 +5507,15 @@ tilegx_function_profiler (FILE *file, int labelno ATTRIBUTE_UNUSED)
>        fprintf (file, "\t}\n");
>      }
>  
> +  if (cfun->static_chain_decl)
> +    {
> +      fprintf (file,
> +	       "\t{\n"
> +	       "\taddi\tsp, sp, -8\n"
> +	       "\tst\tsp, r10\n"
> +	       "\t}\n");
> +    }
> +
>    if (flag_pic)
>      {
>        fprintf (file,
> @@ -5524,6 +5533,13 @@ tilegx_function_profiler (FILE *file, int labelno ATTRIBUTE_UNUSED)
>  	       "\t}\n", MCOUNT_NAME);
>      }
>  
> +  if (cfun->static_chain_decl)
> +    {
> +      fprintf (file,
> +	       "\taddi\tsp, sp, 8\n"
> +	       "\tld\tr10, sp\n");
> +    }
> +
>    tilegx_in_bundle = false;
>  }
>  
> 

-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.

-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.

Re: [PATCH v3] gcc/config/tilegx/tilegx.c (tilegx_function_profiler): Save r10 to stack before call mcount

[Jeff Law]

On 10/06/2016 07:53 AM, Chen Gang wrote:
> Hello Maintainers:
>
> Is this patch OK? Please help check it when you have time (at least for
> me, it passes all related test and comparation).
>
> And I shall continue to find and fix another issues about tilegx.
>
> Thanks.
>
> On 6/4/16 21:25, chengang@emindsoft.com.cn wrote:
>> From: Chen Gang <gang.chen.5i5j@gmail.com>
>>
>> r10 may also be as parameter stack pointer for the nested function, so
>> need save it before call mcount.
>>
>> Also clean up code: use '!' instead of "== 0" for checking
>> static_chain_decl and compute_total_frame_size.
>>
>> 2016-06-04  Chen Gang  <gang.chen.5i5j@gmail.com>
>>
>> 	gcc/
>> 	PR target/71331
>> 	* config/tilegx/tilegx.c (tilegx_function_profiler): Save r10
>> 	to stack before call mcount.
>> 	(tilegx_can_use_return_insn_p): Clean up code.
So if I understand the tilegx architecture correctly, you're issuing the 
r10 save & sp adjustment as a bundle, and the restore & sp adjustment as 
a bundle.

The problem is the semantics of bunding on the tilegx effectively mean 
that all source operands are read in parallel, then all outputs occur in 
parallel.

So if we take the bundle

{addi sp,sp,-8 ; st sp, r10}

The address used for the st is the value of the stack pointer before the 
addi instruction.

Similarly for the restore r10 bundle.  The address used for the load is 
sp before adjustment.

Given my understanding of the tilegx bundling semantics, that seems wrong.

Jeff

Re: [PATCH v3] gcc/config/tilegx/tilegx.c (tilegx_function_profiler): Save r10 to stack before call mcount

[Chen Gang]

On 10/20/16 06:42, Jeff Law wrote:
>> On 6/4/16 21:25, chengang@emindsoft.com.cn wrote:
>>> From: Chen Gang <gang.chen.5i5j@gmail.com>
>>>
>>> r10 may also be as parameter stack pointer for the nested function, so
>>> need save it before call mcount.
>>>
>>> Also clean up code: use '!' instead of "== 0" for checking
>>> static_chain_decl and compute_total_frame_size.
>>>
>>> 2016-06-04  Chen Gang  <gang.chen.5i5j@gmail.com>
>>>
>>>     gcc/
>>>     PR target/71331
>>>     * config/tilegx/tilegx.c (tilegx_function_profiler): Save r10
>>>     to stack before call mcount.
>>>     (tilegx_can_use_return_insn_p): Clean up code.
> So if I understand the tilegx architecture correctly, you're issuing the r10 save & sp adjustment as a bundle, and the restore & sp adjustment as a bundle.
> 
> The problem is the semantics of bunding on the tilegx effectively mean that all source operands are read in parallel, then all outputs occur in parallel.
> 
> So if we take the bundle
> 
> {addi sp,sp,-8 ; st sp, r10}
> 
> The address used for the st is the value of the stack pointer before the addi instruction.
> 
> Similarly for the restore r10 bundle.  The address used for the load is sp before adjustment.
> 
> Given my understanding of the tilegx bundling semantics, that seems wrong.
> 
> Jeff
>
 
The comments on 1st page of "TILE-Gx Instruction Set Architecture":

Individual instructions within a bundle must comply with certain register semantics. Read-after-write (RAW) dependencies are enforced between instruction bundles. There is no ordering within a bundle, and the numbering of pipelines or instruction slots within a bundle is only used for convenience and does not imply any ordering. Within an instruction bundle, it is valid to encode an output operand that is the same as an input operand. Because there is explicitly no implied dependency within a bundle, the semantics for this specify that the input operands for all instructions in a bundle are read before any of the output operands are written.

Write-after-write (WAW) semantics between two bundles are defined as: the latest write over-writes earlier writes.

Within a bundle, WAW dependencies are forbidden. If more than one instruction in a bundle writes to the same output operand register, unpredictable results for any destination operand within that bundle can occur. Also, implementations are free to signal this case as an illegal instruction. There is one exception to this rule—multiple instructions within a bundle may legally target the zero register. Lastly, some instructions, such as instructions that implicitly write the link register, implicitly write registers. If an instruction implicitly writes to a register that another instruction in the same bundle writes to, unpredictable results can occur for any output register used by that bundle and/or an illegal instruction interrupt can occur.

On Page 221, ld instruction is:

  ld Dest, Src

On Page 251, st instruction is:

  st SrcA, SrcB


So for me:

  Bundle {addi sp, sp, 8; ld r10, sp} is OK, it is RAW.

  Bundle {addi sp, sp, -8; st sp, r10} is OK, too, it is RAW (not WAW --
  both SrcA and SrcB are input operands).


Please help check, if need the related document, please let me know.

Thanks.
-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.

Re: [PATCH v3] gcc/config/tilegx/tilegx.c (tilegx_function_profiler): Save r10 to stack before call mcount

[Chris Metcalf]

On 10/21/2016 6:24 PM, Chen Gang wrote:
> On 10/20/16 06:42, Jeff Law wrote:
>>> On 6/4/16 21:25, chengang@emindsoft.com.cn wrote:
>>>> From: Chen Gang <gang.chen.5i5j@gmail.com>
>>>>
>>>> r10 may also be as parameter stack pointer for the nested function, so
>>>> need save it before call mcount.
>>>>
>>>> Also clean up code: use '!' instead of "== 0" for checking
>>>> static_chain_decl and compute_total_frame_size.
>>>>
>>>> 2016-06-04  Chen Gang  <gang.chen.5i5j@gmail.com>
>>>>
>>>>      gcc/
>>>>      PR target/71331
>>>>      * config/tilegx/tilegx.c (tilegx_function_profiler): Save r10
>>>>      to stack before call mcount.
>>>>      (tilegx_can_use_return_insn_p): Clean up code.
>> So if I understand the tilegx architecture correctly, you're issuing the r10 save & sp adjustment as a bundle, and the restore & sp adjustment as a bundle.
>>
>> The problem is the semantics of bunding on the tilegx effectively mean that all source operands are read in parallel, then all outputs occur in parallel.
>>
>> So if we take the bundle
>>
>> {addi sp,sp,-8 ; st sp, r10}
>>
>> The address used for the st is the value of the stack pointer before the addi instruction.
>>
>> Similarly for the restore r10 bundle.  The address used for the load is sp before adjustment.
>>
>> Given my understanding of the tilegx bundling semantics, that seems wrong.
>>
>> Jeff
>>
>   
> The comments on 1st page of "TILE-Gx Instruction Set Architecture":
>
> Individual instructions within a bundle must comply with certain register semantics. Read-after-write (RAW) dependencies are enforced between instruction bundles. There is no ordering within a bundle, and the numbering of pipelines or instruction slots within a bundle is only used for convenience and does not imply any ordering. Within an instruction bundle, it is valid to encode an output operand that is the same as an input operand. Because there is explicitly no implied dependency within a bundle, the semantics for this specify that the input operands for all instructions in a bundle are read before any of the output operands are written.
>
> Write-after-write (WAW) semantics between two bundles are defined as: the latest write over-writes earlier writes.
>
> Within a bundle, WAW dependencies are forbidden. If more than one instruction in a bundle writes to the same output operand register, unpredictable results for any destination operand within that bundle can occur. Also, implementations are free to signal this case as an illegal instruction. There is one exception to this rule—multiple instructions within a bundle may legally target the zero register. Lastly, some instructions, such as instructions that implicitly write the link register, implicitly write registers. If an instruction implicitly writes to a register that another instruction in the same bundle writes to, unpredictable results can occur for any output register used by that bundle and/or an illegal instruction interrupt can occur.
>
> On Page 221, ld instruction is:
>
>    ld Dest, Src
>
> On Page 251, st instruction is:
>
>    st SrcA, SrcB
>
>
> So for me:
>
>    Bundle {addi sp, sp, 8; ld r10, sp} is OK, it is RAW.
>
>    Bundle {addi sp, sp, -8; st sp, r10} is OK, too, it is RAW (not WAW --
>    both SrcA and SrcB are input operands).
>
>
> Please help check, if need the related document, please let me know.

As you wrote, RAW applies "between instruction bundles".  In this case you are looking at register usage within a single bundle, and as you wrote, "the input operands for all instructions in a bundle are read before any of the output operands are written."  So for your two bundles quoted above, the "sp" input operand for both instructions will have the same value, i.e. the load/store will have the pre-adjusted "sp" value.

-- 
Chris Metcalf, Mellanox Technologies
http://www.mellanox.com