diff mbox

[COMMITTED,AArch64] Improve TLS Descriptor pattern to release RTL loop IV opt

Message ID n99mvy4s8d9.fsf@arm.com
State New
Headers show

Commit Message

Jiong Wang Aug. 6, 2015, 4:10 p.m. UTC 
James Greenhalgh writes:

> On Tue, Jul 28, 2015 at 02:12:36PM +0100, Jiong Wang wrote:
>> 
>> The instruction sequences for preparing argument for TLS descriptor
>> runtime resolver and the later function call to resolver can actually be
>> hoisted out of the loop.
>> 
>> Currently we can't because we have exposed the hard register X0 as
>> destination of "set".  While GCC's RTL data flow infrastructure will
>> skip or do very conservative assumption when hard register involved in
>> and thus some loop IV opportunities are missed.
>> 
>> This patch add another "tlsdesc_small_pseudo_<mode>" pattern, and avoid
>> expose x0 to gcc generic code.
>> 
>> Generally, we define a new register class FIXED_R0 which only contains register
>> 0, so the instruction sequences generated from the new add pattern is the same
>> as tlsdesc_small_<mode>, while the operand 0 is wrapped as pseudo register that
>> RTL IV opt can handle it.
>> 
>> Ideally, we should allow operand 0 to be any pseudo register, but then
>> we can't model the override of x0 caused by the function call which is
>> hidded by the UNSPEC.
>> 
>> So here, we restricting operand 0 to be x0, the override of x0 can be
>> reflected to the gcc.
>> 
>> OK for trunk?
>
> OK.
>
> Thanks,
> James

When I am about to commit this patch, I realized I need to apply the
same trick as I have done at

  https://gcc.gnu.org/ml/gcc-patches/2015-07/msg01653.html

then the included testcases can work well on any of tiny, small, large model.

commited the following patch:

Comments

Andreas Schwab Aug. 8, 2015, 11:59 a.m. UTC | #1 
Jiong Wang <jiong.wang@arm.com> writes:

> Index: gcc/ChangeLog
> ===================================================================
> --- gcc/ChangeLog	(revision 226682)
> +++ gcc/ChangeLog	(working copy)
> @@ -1,3 +1,16 @@
> +2015-08-06    Ramana Radhakrishnan  <ramana.radhakrishnan@arm.com>
> +	      Jiong Wang  <jiong.wang@arm.com>
> +
> +	* config/aarch64/aarch64.d (tlsdesc_small_pseudo_<mode>): New pattern.
> +	* config/aarch64/aarch64.h (reg_class): New enumeration FIXED_REG0.
> +	(REG_CLASS_NAMES): Likewise.
> +	(REG_CLASS_CONTENTS): Likewise.
> +	* config/aarch64/aarch64.c (aarch64_class_max_nregs): Likewise.
> +	(aarch64_register_move_cost): Likewise.
> +	(aarch64_load_symref_appropriately): Invoke the new added pattern if
> +	possible.
> +	* config/aarch64/constraints.md (Uc0): New constraint.

That breaks go, all tests are crashing now.

Andreas.
Jiong Wang Aug. 10, 2015, 9:34 a.m. UTC | #2 
Andreas Schwab writes:

> Jiong Wang <jiong.wang@arm.com> writes:
>
>> Index: gcc/ChangeLog
>> ===================================================================
>> --- gcc/ChangeLog	(revision 226682)
>> +++ gcc/ChangeLog	(working copy)
>> @@ -1,3 +1,16 @@
>> +2015-08-06    Ramana Radhakrishnan  <ramana.radhakrishnan@arm.com>
>> +	      Jiong Wang  <jiong.wang@arm.com>
>> +
>> +	* config/aarch64/aarch64.d (tlsdesc_small_pseudo_<mode>): New pattern.
>> +	* config/aarch64/aarch64.h (reg_class): New enumeration FIXED_REG0.
>> +	(REG_CLASS_NAMES): Likewise.
>> +	(REG_CLASS_CONTENTS): Likewise.
>> +	* config/aarch64/aarch64.c (aarch64_class_max_nregs): Likewise.
>> +	(aarch64_register_move_cost): Likewise.
>> +	(aarch64_load_symref_appropriately): Invoke the new added pattern if
>> +	possible.
>> +	* config/aarch64/constraints.md (Uc0): New constraint.
>
> That breaks go, all tests are crashing now.

Andreas,

  Thanks for the information.

  * I found I committed the wrong patch!
    there are two patches in my local directory, one is
    "tlsdesc_hoist.patch" the other is "tlsdesc-hoist.patch", the one
    approved and up-to-date is tlsdesc-hoist.patch while I committed
    tlsdesc_hoist.patch.
    
    Reverted the wrong commit and committed the correct/approved
    version.

  * Even after the correct patch applied, I still found go check failed
    on my local native check.

    Tring to understand why and if I can't figure out today I will
    revert the patch.
    
  Sorry about the trouble!
Jiong Wang Aug. 10, 2015, 1:04 p.m. UTC | #3 
Jiong Wang writes:

> Andreas Schwab writes:
>
>> Jiong Wang <jiong.wang@arm.com> writes:
>>
>>> Index: gcc/ChangeLog
>>> ===================================================================
>>> --- gcc/ChangeLog	(revision 226682)
>>> +++ gcc/ChangeLog	(working copy)
>>> @@ -1,3 +1,16 @@
>>> +2015-08-06    Ramana Radhakrishnan  <ramana.radhakrishnan@arm.com>
>>> +	      Jiong Wang  <jiong.wang@arm.com>
>>> +
>>> +	* config/aarch64/aarch64.d (tlsdesc_small_pseudo_<mode>): New pattern.
>>> +	* config/aarch64/aarch64.h (reg_class): New enumeration FIXED_REG0.
>>> +	(REG_CLASS_NAMES): Likewise.
>>> +	(REG_CLASS_CONTENTS): Likewise.
>>> +	* config/aarch64/aarch64.c (aarch64_class_max_nregs): Likewise.
>>> +	(aarch64_register_move_cost): Likewise.
>>> +	(aarch64_load_symref_appropriately): Invoke the new added pattern if
>>> +	possible.
>>> +	* config/aarch64/constraints.md (Uc0): New constraint.
>>
>> That breaks go, all tests are crashing now.
>
> Andreas,
>
>   Thanks for the information.
>
>   * I found I committed the wrong patch!
>     there are two patches in my local directory, one is
>     "tlsdesc_hoist.patch" the other is "tlsdesc-hoist.patch", the one
>     approved and up-to-date is tlsdesc-hoist.patch while I committed
>     tlsdesc_hoist.patch.
>     
>     Reverted the wrong commit and committed the correct/approved
>     version.
>
>   * Even after the correct patch applied, I still found go check failed
>     on my local native check.
>
>     Tring to understand why and if I can't figure out today I will
>     revert the patch.

And I just finished two round of native aarch64 build/check w/wo my patch.

I got the same go.sum.

And my patch only touches one tls descriptor pattern which will only be
used if there is tls variable. So I suspect the go test regressions are
not caused by my patch.

Andreas, can you please double confirm this?

Thanks
Andreas Schwab Aug. 10, 2015, 1:20 p.m. UTC | #4 
Jiong Wang <jiong.wang@arm.com> writes:

> And I just finished two round of native aarch64 build/check w/wo my patch.

Did you rebuild everything?

Andreas.
Jiong Wang Aug. 10, 2015, 1:26 p.m. UTC | #5 
Andreas Schwab writes:

> Jiong Wang <jiong.wang@arm.com> writes:
>
>> And I just finished two round of native aarch64 build/check w/wo my patch.
>
> Did you rebuild everything?

No.

Just applied the patch, then "make all" and re-check

>
> Andreas.
Andreas Schwab Aug. 10, 2015, 1:33 p.m. UTC | #6 
Jiong Wang <jiong.wang@arm.com> writes:

> Andreas Schwab writes:
>
>> Jiong Wang <jiong.wang@arm.com> writes:
>>
>>> And I just finished two round of native aarch64 build/check w/wo my patch.
>>
>> Did you rebuild everything?
>
> No.

Please do.

Andreas.
Jiong Wang Aug. 10, 2015, 10:49 p.m. UTC | #7 
Andreas Schwab writes:

> Jiong Wang <jiong.wang@arm.com> writes:
>
>> Andreas Schwab writes:
>>
>>> Jiong Wang <jiong.wang@arm.com> writes:
>>>
>>>> And I just finished two round of native aarch64 build/check w/wo my patch.
>>>
>>> Did you rebuild everything?
>>
>> No.
>
> Please do.

Andreas,

  I Just finished several round of rebuild & testing on clean
  environment.

  The problem should have gone away after my new tlsdesc patch.  

  While I do have seen lots of failures with my old tlsdesc patch.

  Please let me know if you still have problem on this after my patch
  correction.
  
  Thanks.
Andreas Schwab Aug. 11, 2015, 7:11 a.m. UTC | #8 
Jiong Wang <jiong.wang@arm.com> writes:

>   I Just finished several round of rebuild & testing on clean
>   environment.

How did you even manage to compile it?

../../gcc/ira.c: In function 'void print_translated_classes(FILE*, bool)':
../../gcc/ira.c:1415:49: error: iteration 8u invokes undefined behavior [-Werror=aggressive-loop-optimizations]
     fprintf (f, " %s -> %s\n", reg_class_names[i],
                                                 ^
../../gcc/ira.c:1414:17: note: containing loop
   for (i = 0; i < N_REG_CLASSES; i++)
                 ^

Andreas.
Jiong Wang Aug. 11, 2015, 8:28 a.m. UTC | #9 
Andreas Schwab writes:

> Jiong Wang <jiong.wang@arm.com> writes:
>
>>   I Just finished several round of rebuild & testing on clean
>>   environment.
>
> How did you even manage to compile it?
>
> ../../gcc/ira.c: In function 'void print_translated_classes(FILE*, bool)':
> ../../gcc/ira.c:1415:49: error: iteration 8u invokes undefined behavior [-Werror=aggressive-loop-optimizations]
>      fprintf (f, " %s -> %s\n", reg_class_names[i],
>                                                  ^
> ../../gcc/ira.c:1414:17: note: containing loop
>    for (i = 0; i < N_REG_CLASSES; i++)
>                  ^
>
> Andreas.

What I have done on a clean environment:

  1. checkout e5c427f3245ff69d8b014d4ff170715962178151 which might be
  the commit cause the trouble.

     mkdir build-dir1

     configure/build/check-go in build-dir1
     (configuation options --enable-languages=c,c++,fortran,go
     --prefix=/home/jiowan01/COMMUNITY/install-gcc-native-reference
     --disable-bootstrap)

     and I do see lots of go failures.
     
  2. checkout the one commit before this wrong commit.

     re-done everything in a new build-dir2

     check-go OK

  3. applied my new tlsdesc patch

     re-done everything in a new build-dir3
     check-go OK

  4. checkout

     commit 8b221598008f60621859caee561e8466e80049f7
     Author: jiwang <jiwang@138bc75d-0d04-0410-961f-82ee72b054a4>
     Date:   Mon Aug 10 10:06:28 2015 +0000

    [AArch64] Recommit correct version for improving TLS descriptor
    pattern

    re-done everything in a new build-dir4
    check-go OK


  my host gcc is "gcc version 4.8.2 (Ubuntu/Linaro 4.8.2-19ubuntu1)", I
  guess you have enabled bootstrap.
Jiong Wang Aug. 11, 2015, 8:42 a.m. UTC | #10 
Andreas Schwab writes:

> Jiong Wang <jiong.wang@arm.com> writes:
>
>>   I Just finished several round of rebuild & testing on clean
>>   environment.
>
> How did you even manage to compile it?
>
> ../../gcc/ira.c: In function 'void print_translated_classes(FILE*, bool)':
> ../../gcc/ira.c:1415:49: error: iteration 8u invokes undefined behavior [-Werror=aggressive-loop-optimizations]
>      fprintf (f, " %s -> %s\n", reg_class_names[i],
>                                                  ^
> ../../gcc/ira.c:1414:17: note: containing loop
>    for (i = 0; i < N_REG_CLASSES; i++)

And, my new patch missed a ',' after "FIXED_REG0", sorry about
that.. that should be the problem for the bootstrap issue.


>
> Andreas.
diff mbox

Patch

Index: gcc/ChangeLog
===================================================================
--- gcc/ChangeLog	(revision 226682)
+++ gcc/ChangeLog	(working copy)
@@ -1,3 +1,16 @@ 
+2015-08-06    Ramana Radhakrishnan  <ramana.radhakrishnan@arm.com>
+	      Jiong Wang  <jiong.wang@arm.com>
+
+	* config/aarch64/aarch64.d (tlsdesc_small_pseudo_<mode>): New pattern.
+	* config/aarch64/aarch64.h (reg_class): New enumeration FIXED_REG0.
+	(REG_CLASS_NAMES): Likewise.
+	(REG_CLASS_CONTENTS): Likewise.
+	* config/aarch64/aarch64.c (aarch64_class_max_nregs): Likewise.
+	(aarch64_register_move_cost): Likewise.
+	(aarch64_load_symref_appropriately): Invoke the new added pattern if
+	possible.
+	* config/aarch64/constraints.md (Uc0): New constraint.
+
 2015-08-06  Jiong Wang  <jiong.wang@arm.com>
 
 	* config/aarch64/constraints.md (Usf): Add the test of
Index: gcc/config/aarch64/aarch64.c
===================================================================
--- gcc/config/aarch64/aarch64.c	(revision 226681)
+++ gcc/config/aarch64/aarch64.c	(working copy)
@@ -1048,12 +1048,26 @@ 
 
 	gcc_assert (mode == Pmode || mode == ptr_mode);
 
-	/* In ILP32, the got entry is always of SImode size.  Unlike
-	   small GOT, the dest is fixed at reg 0.  */
-	if (TARGET_ILP32)
-	  emit_insn (gen_tlsdesc_small_si (imm));
+	if (can_create_pseudo_p ())
+	  {
+	    rtx reg = gen_reg_rtx (mode);
+
+	    if (TARGET_ILP32)
+	      emit_insn (gen_tlsdesc_small_pseudo_si (imm, reg));
+	    else
+	      emit_insn (gen_tlsdesc_small_pseudo_di (imm, reg));
+
+	    emit_use (reg);
+	  }
 	else
-	  emit_insn (gen_tlsdesc_small_di (imm));
+	  {
+	    /* In ILP32, the got entry is always of SImode size.  Unlike
+	       small GOT, the dest is fixed at reg 0.  */
+	    if (TARGET_ILP32)
+	      emit_insn (gen_tlsdesc_small_si (imm));
+	    else
+	      emit_insn (gen_tlsdesc_small_di (imm));
+	  }
 	tp = aarch64_load_tp (NULL);
 
 	if (mode != Pmode)
Index: gcc/config/aarch64/aarch64.md
===================================================================
--- gcc/config/aarch64/aarch64.md	(revision 226681)
+++ gcc/config/aarch64/aarch64.md	(working copy)
@@ -4549,6 +4549,23 @@ 
   [(set_attr "type" "call")
    (set_attr "length" "16")])
 
+;; The same as tlsdesc_small_<mode> except that we don't expose hard register X0
+;; as the destination of set as it will cause trouble for RTL loop iv.
+;; RTL loop iv will abort ongoing optimization once it finds there is hard reg
+;; as destination of set.  This pattern thus could help these tlsdesc
+;; instruction sequences hoisted out of loop.
+(define_insn "tlsdesc_small_pseudo_<mode>"
+  [(set (match_operand:PTR 1 "register_operand" "=r")
+        (unspec:PTR [(match_operand 0 "aarch64_valid_symref" "S")]
+		   UNSPEC_TLSDESC))
+   (clobber (reg:DI R0_REGNUM))
+   (clobber (reg:DI LR_REGNUM))
+   (clobber (reg:CC CC_REGNUM))]
+  "TARGET_TLS_DESC"
+  "adrp\\tx0, %A0\;ldr\\t%<w>1, [x0, #%L0]\;add\\t<w>0, <w>0, %L0\;.tlsdesccall\\t%0\;blr\\t%1"
+  [(set_attr "type" "call")
+   (set_attr "length" "16")])
+
 (define_insn "stack_tie"
   [(set (mem:BLK (scratch))
 	(unspec:BLK [(match_operand:DI 0 "register_operand" "rk")
Index: gcc/testsuite/ChangeLog
===================================================================
--- gcc/testsuite/ChangeLog	(revision 226682)
+++ gcc/testsuite/ChangeLog	(working copy)
@@ -1,5 +1,9 @@ 
 2015-08-06  Jiong Wang  <jiong.wang@arm.com>
 
+	* gcc.target/aarch64/tlsdesc_hoist.c: New testcase.
+
+2015-08-06  Jiong Wang  <jiong.wang@arm.com>
+
 	* gcc.target/aarch64/noplt_3.c: New testcase.
 
 2015-08-06  Jiong Wang  <jiong.wang@arm.com>
Index: gcc/testsuite/gcc.target/aarch64/tlsdesc_hoist.c
===================================================================
--- gcc/testsuite/gcc.target/aarch64/tlsdesc_hoist.c	(revision 0)
+++ gcc/testsuite/gcc.target/aarch64/tlsdesc_hoist.c	(working copy)
@@ -0,0 +1,22 @@ 
+/* { dg-do compile } */
+/* { dg-require-effective-target tls_native } */
+/* { dg-options "-O2 -fpic -fdump-rtl-loop2_invariant" } */
+/* { dg-skip-if "-mcmodel=large, no support for -fpic" { aarch64-*-* }  { "-mcmodel=large" } { "" } } */
+
+int cal (int, int);
+__thread int tls_data;
+
+int
+foo (int bound)
+{
+  int i = 0;
+  int sum = 0;
+
+  for (i; i < bound; i++)
+    sum = cal (sum, tls_data);
+
+  return sum;
+}
+
+/* Insn sequences for TLS descriptor should be hoisted out of the loop.  */
+/* { dg-final { scan-rtl-dump "Decided" "loop2_invariant" } } */