Fix PR 49014

public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed

* Fix PR 49014
@ 2011-05-25  9:45 Andrey Belevantsev
  2011-05-25 14:57 ` Bernd Schmidt
  0 siblings, 1 reply; 10+ messages in thread
From: Andrey Belevantsev @ 2011-05-25  9:45 UTC (permalink / raw)
  To: GCC Patches; +Cc: Vladimir N. Makarov, Bernd Schmidt

Hello,

This patch fixes PR 49014, yet another case of the insn with wrong 
reservation.  Approved by Uros in the PR audit trail, bootstrapped and 
regtested on x86-64/linux and committed to trunk.

Vlad, Bernd, I wonder if we can avoid having recog_memoized >=0 insns that 
do not have proper DFA reservations (that is, they do not change the DFA 
state).  I see that existing practice allows this as shown by Bernd's patch 
to 48403, i.e. such insns do not count against issue_rate.  I would be 
happy to fix sel-sched in the same way.  However, both sel-sched ICEs as 
shown by PRs 48143 and 49014 really uncover the latent bugs in the backend. 
  So, is it possible to stop having such insns if scheduling is desired, or 
otherwise distinguish the insns that wrongly miss the proper DFA reservation?

Yours, Andrey

Index: gcc/ChangeLog
===================================================================
*** gcc/ChangeLog       (revision 174171)
--- gcc/ChangeLog       (working copy)
***************
*** 1,3 ****
--- 1,8 ----
+ 2011-05-25  Andrey Belevantsev  <abel@ispras.ru>
+
+       PR rtl-optimization/49014
+       * config/i386/athlon.md (athlon_ssecomi): Change type to ssecomi.
+
   2011-05-25  Jakub Jelinek  <jakub@redhat.com>

         PR target/49128
Index: gcc/config/i386/athlon.md
===================================================================
*** gcc/config/i386/athlon.md   (revision 174171)
--- gcc/config/i386/athlon.md   (working copy)
*************** (define_insn_reservation "athlon_ssecomi
*** 798,804 ****
                          "athlon-direct,athlon-fploadk8,athlon-fadd")
   (define_insn_reservation "athlon_ssecomi" 4
                          (and (eq_attr "cpu" "athlon,k8,generic64")
!                             (eq_attr "type" "ssecmp"))
                          "athlon-vector,athlon-fpsched,athlon-fadd")
   (define_insn_reservation "athlon_ssecomi_amdfam10" 3
                          (and (eq_attr "cpu" "amdfam10")
--- 798,804 ----
                          "athlon-direct,athlon-fploadk8,athlon-fadd")
   (define_insn_reservation "athlon_ssecomi" 4
                          (and (eq_attr "cpu" "athlon,k8,generic64")
!                             (eq_attr "type" "ssecomi"))
                          "athlon-vector,athlon-fpsched,athlon-fadd")
   (define_insn_reservation "athlon_ssecomi_amdfam10" 3
                          (and (eq_attr "cpu" "amdfam10")

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: Fix PR 49014
  2011-05-25  9:45 Fix PR 49014 Andrey Belevantsev
@ 2011-05-25 14:57 ` Bernd Schmidt
  2011-05-25 16:42   ` Andrey Belevantsev
  0 siblings, 1 reply; 10+ messages in thread
From: Bernd Schmidt @ 2011-05-25 14:57 UTC (permalink / raw)
  To: Andrey Belevantsev; +Cc: GCC Patches, Vladimir N. Makarov

On 05/25/2011 08:21 AM, Andrey Belevantsev wrote:
> Vlad, Bernd, I wonder if we can avoid having recog_memoized >=0 insns
> that do not have proper DFA reservations (that is, they do not change
> the DFA state).  I see that existing practice allows this as shown by
> Bernd's patch to 48403, i.e. such insns do not count against
> issue_rate.  I would be happy to fix sel-sched in the same way. 
> However, both sel-sched ICEs as shown by PRs 48143 and 49014 really
> uncover the latent bugs in the backend.  So, is it possible to stop
> having such insns if scheduling is desired, or otherwise distinguish the
> insns that wrongly miss the proper DFA reservation?

Add a bool target podhook, targetm.sched.all_insns_have_reservations,
and add an assert in the scheduler if it is true.

I'm not sure what a good default value would be. Defining it to true
would almost certainly break a few ports initially (even assuming we
override it in sh where it's known not to be true), but I guess it such
an assertion failure would be useful information for the target maintainers.

Or, if we want to enable extra checking on ports where not all insns
have a reservation, a new insn attribute ("has_reservation") could be
defined, defined to evaluate to true by default in genattrtab, and
(set_attr "has_reservation" "0") added in the machine descriptions where
necessary.

Bernd

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: Fix PR 49014
  2011-05-25 14:57 ` Bernd Schmidt
@ 2011-05-25 16:42   ` Andrey Belevantsev
  2011-05-25 16:49     ` Bernd Schmidt
  0 siblings, 1 reply; 10+ messages in thread
From: Andrey Belevantsev @ 2011-05-25 16:42 UTC (permalink / raw)
  To: Bernd Schmidt; +Cc: GCC Patches, Vladimir N. Makarov

On 25.05.2011 18:41, Bernd Schmidt wrote:
>
> On 05/25/2011 08:21 AM, Andrey Belevantsev wrote:
>> Vlad, Bernd, I wonder if we can avoid having recog_memoized>=0 insns
>> that do not have proper DFA reservations (that is, they do not change
>> the DFA state).  I see that existing practice allows this as shown by
>> Bernd's patch to 48403, i.e. such insns do not count against
>> issue_rate.  I would be happy to fix sel-sched in the same way.
>> However, both sel-sched ICEs as shown by PRs 48143 and 49014 really
>> uncover the latent bugs in the backend.  So, is it possible to stop
>> having such insns if scheduling is desired, or otherwise distinguish the
>> insns that wrongly miss the proper DFA reservation?
>
> Add a bool target podhook, targetm.sched.all_insns_have_reservations,
> and add an assert in the scheduler if it is true.
>
> I'm not sure what a good default value would be. Defining it to true
> would almost certainly break a few ports initially (even assuming we
> override it in sh where it's known not to be true), but I guess it such
> an assertion failure would be useful information for the target maintainers.
I think the hook is a better idea than the attribute because nobody will 
care to mark all offending insns with an attribute.  However, I must take a 
look to see how much work should be done for x86-64 to make the hook return 
true by default.  I didn't get even past the very first libgcc build with 
this assert enabled.

Andrey

>
> Or, if we want to enable extra checking on ports where not all insns
> have a reservation, a new insn attribute ("has_reservation") could be
> defined, defined to evaluate to true by default in genattrtab, and
> (set_attr "has_reservation" "0") added in the machine descriptions where
> necessary.
>
>
> Bernd

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: Fix PR 49014
  2011-05-25 16:42   ` Andrey Belevantsev
@ 2011-05-25 16:49     ` Bernd Schmidt
  2011-05-26 13:53       ` Andrey Belevantsev
  0 siblings, 1 reply; 10+ messages in thread
From: Bernd Schmidt @ 2011-05-25 16:49 UTC (permalink / raw)
  To: Andrey Belevantsev; +Cc: GCC Patches, Vladimir N. Makarov

On 05/25/2011 03:29 PM, Andrey Belevantsev wrote:
> I think the hook is a better idea than the attribute because nobody will
> care to mark all offending insns with an attribute.

I don't know. IIRC when I looked at sh or whatever the broken port was,
it was only two insns - there would still be some value in being able to
assert that all other insns have a reservation.


Bernd

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: Fix PR 49014
  2011-05-25 16:49     ` Bernd Schmidt
@ 2011-05-26 13:53       ` Andrey Belevantsev
  2011-07-01 14:50         ` Andrey Belevantsev
  0 siblings, 1 reply; 10+ messages in thread
From: Andrey Belevantsev @ 2011-05-26 13:53 UTC (permalink / raw)
  To: Bernd Schmidt; +Cc: GCC Patches, Vladimir N. Makarov

On 25.05.2011 19:31, Bernd Schmidt wrote:
> On 05/25/2011 03:29 PM, Andrey Belevantsev wrote:
>> I think the hook is a better idea than the attribute because nobody will
>> care to mark all offending insns with an attribute.
>
> I don't know. IIRC when I looked at sh or whatever the broken port was,
> it was only two insns - there would still be some value in being able to
> assert that all other insns have a reservation.
OK, I will take a look on x86-64 and will get back with more information.

Andrey

>
>
> Bernd

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: Fix PR 49014
  2011-05-26 13:53       ` Andrey Belevantsev
@ 2011-07-01 14:50         ` Andrey Belevantsev
  2011-07-07 16:28           ` Vladimir Makarov
  2011-07-07 17:13           ` Bernd Schmidt
  0 siblings, 2 replies; 10+ messages in thread
From: Andrey Belevantsev @ 2011-07-01 14:50 UTC (permalink / raw)
  To: Bernd Schmidt; +Cc: GCC Patches, Vladimir N. Makarov

[-- Attachment #1: Type: text/plain, Size: 1375 bytes --]

On 26.05.2011 17:32, Andrey Belevantsev wrote:
> On 25.05.2011 19:31, Bernd Schmidt wrote:
>> On 05/25/2011 03:29 PM, Andrey Belevantsev wrote:
>>> I think the hook is a better idea than the attribute because nobody will
>>> care to mark all offending insns with an attribute.
>>
>> I don't know. IIRC when I looked at sh or whatever the broken port was,
>> it was only two insns - there would still be some value in being able to
>> assert that all other insns have a reservation.
> OK, I will take a look on x86-64 and will get back with more information.
>
> Andrey
So, I have made an attempt to bootstrap on x86-64 with the extra assert in 
selective scheduling that assumes the DFA state always changes when issuing 
a recog_memoized >=0 insn (patch attached).  Indeed, there are just a few 
general insns that don't have proper reservations.  However, it was a 
surprise to me to see that almost any insn with SSE registers fails this 
assert and thus does not get properly scheduled.

Overall, the work on fixing those seems doable, it took just a day to get 
the compiler bootstrapped (of course, the testsuite may bring much more 
issues).  So, if there is an agreement on marking a few offending insns 
with the new attribute, we can proceed with the help of somebody from the 
x86 land on fixing those and researching for other targets.

Andrey

>
>>
>>
>> Bernd


[-- Attachment #2: nondfa-insn-bootstrap-x86-64.diff --]
[-- Type: text/x-patch, Size: 19684 bytes --]

Index: gcc/sel-sched.c
===================================================================
*** gcc/sel-sched.c	(revision 175757)
--- gcc/sel-sched.c	(working copy)
*************** advance_state_on_fence (fence_t fence, i
*** 5305,5310 ****
--- 5305,5315 ----
            if (FENCE_ISSUED_INSNS (fence) > issue_rate)
              gcc_unreachable ();
          }
+       else if (get_attr_nondfa_insn (insn) == 0)
+ 	{
+ 	  error ("found an insn that does not modify DFA state");
+ 	  fatal_insn ("this is the insn:", insn);
+ 	}
      }
    else
      {
Index: gcc/common.opt
===================================================================
*** gcc/common.opt	(revision 175757)
--- gcc/common.opt	(working copy)
*************** Common Report Var(flag_selective_schedul
*** 1687,1697 ****
  Schedule instructions using selective scheduling algorithm
  
  fselective-scheduling2
! Common Report Var(flag_selective_scheduling2) Optimization 
  Run selective scheduling after reload
  
  fsel-sched-pipelining
! Common Report Var(flag_sel_sched_pipelining) Init(0) Optimization
  Perform software pipelining of inner loops during selective scheduling
  
  fsel-sched-pipelining-outer-loops
--- 1687,1697 ----
  Schedule instructions using selective scheduling algorithm
  
  fselective-scheduling2
! Common Report Var(flag_selective_scheduling2) Optimization Init(1)
  Run selective scheduling after reload
  
  fsel-sched-pipelining
! Common Report Var(flag_sel_sched_pipelining) Init(1) Optimization
  Perform software pipelining of inner loops during selective scheduling
  
  fsel-sched-pipelining-outer-loops
Index: gcc/config/i386/i386.md
===================================================================
*** gcc/config/i386/i386.md	(revision 175757)
--- gcc/config/i386/i386.md	(working copy)
*************** (define_attr "prefix" "orig,vex,maybe_ve
*** 518,523 ****
--- 518,526 ----
  ;; VEX W bit is used.
  (define_attr "prefix_vex_w" "" (const_int 0))
  
+ 
+ (define_attr "nondfa_insn" "" (const_int 0))
+ 
  ;; The length of VEX prefix
  ;; Only instructions with 0f prefix can have 2 byte VEX prefix,
  ;; 0f38/0f3a prefixes can't.  In i386.md 0f3[8a] is
*************** (define_insn "x86_fnstsw_1"
*** 1467,1472 ****
--- 1470,1476 ----
    "fnstsw\t%0"
    [(set (attr "length") (symbol_ref "ix86_attr_length_address_default (insn) + 2"))
     (set_attr "mode" "SI")
+    (set (attr "nondfa_insn") (const_int 1))
     (set_attr "unit" "i387")])
  
  ;; FP compares, step 3
*************** (define_insn "*movti_internal_sse"
*** 1962,1967 ****
--- 1966,1972 ----
  }
    [(set_attr "type" "sselog1,ssemov,ssemov")
     (set_attr "prefix" "maybe_vex")
+    (set (attr "nondfa_insn") (const_int 1))
     (set (attr "mode")
  	(cond [(ior (eq (symbol_ref "TARGET_SSE2") (const_int 0))
  		    (ne (symbol_ref "optimize_function_for_size_p (cfun)")
*************** (define_insn "*movdi_internal"
*** 2152,2157 ****
--- 2157,2163 ----
       (if_then_else (eq_attr "alternative" "9,10,11,12")
         (const_string "noavx")
         (const_string "*")))
+    (set (attr "nondfa_insn") (if_then_else (eq_attr "alternative" "2,3,4,5,6,7,8,9,10,11,12") (const_int 1) (const_int 0)))
     (set (attr "type")
       (cond [(eq_attr "alternative" "0,1")
  	      (const_string "multi")
*************** (define_insn "*movsi_internal"
*** 2244,2249 ****
--- 2250,2256 ----
       (if_then_else (and (eq_attr "type" "ssemov") (eq_attr "mode" "SI"))
         (const_string "1")
         (const_string "*")))
+    (set (attr "nondfa_insn") (if_then_else (eq_attr "alternative" "6,7,9,11") (const_int 1) (const_int 0)))
     (set (attr "mode")
       (cond [(eq_attr "alternative" "2,3")
  	      (const_string "DI")
*************** (define_insn "*movtf_internal"
*** 2862,2867 ****
--- 2869,2875 ----
  }
    [(set_attr "type" "ssemov,ssemov,sselog1,*,*")
     (set_attr "prefix" "maybe_vex,maybe_vex,maybe_vex,*,*")
+    (set (attr "nondfa_insn") (if_then_else (eq_attr "alternative" "0,1,2") (const_int 1) (const_int 0)))
     (set (attr "mode")
          (cond [(eq_attr "alternative" "0,2")
  		 (if_then_else
*************** (define_insn "*movdf_internal"
*** 3113,3118 ****
--- 3121,3127 ----
      }
  }
    [(set_attr "type" "fmov,fmov,fmov,multi,multi,sselog1,ssemov,ssemov,ssemov")
+    (set (attr "nondfa_insn") (if_then_else (eq_attr "alternative" "3,4,5,6,7,8") (const_int 1) (const_int 0)))
     (set (attr "prefix")
       (if_then_else (eq_attr "alternative" "0,1,2,3,4")
         (const_string "orig")
*************** (define_insn "zero_extendsidi2_1"
*** 3423,3428 ****
--- 3432,3438 ----
     %vmovd\t{%1, %0|%0, %1}
     %vmovd\t{%1, %0|%0, %1}"
    [(set_attr "type" "multi,multi,multi,mmxmov,mmxmov,ssemov,ssemov")
+    (set (attr "nondfa_insn") (if_then_else (eq_attr "alternative" "3,4,5,6") (const_int 1) (const_int 0)))
     (set_attr "prefix" "*,*,*,orig,orig,maybe_vex,maybe_vex")
     (set_attr "mode" "SI,SI,SI,DI,DI,TI,TI")])
  
*************** (define_insn "*truncdfsf_fast_sse"
*** 4041,4046 ****
--- 4051,4057 ----
    "TARGET_SSE2 && TARGET_SSE_MATH"
    "%vcvtsd2ss\t{%1, %d0|%d0, %1}"
    [(set_attr "type" "ssecvt")
+    (set (attr "nondfa_insn") (const_int 1))
     (set_attr "prefix" "maybe_vex")
     (set_attr "mode" "SF")])
  
*************** (define_insn "fix_trunc<mode>si_sse"
*** 4374,4379 ****
--- 4385,4391 ----
    "%vcvtt<ssemodesuffix>2si\t{%1, %0|%0, %1}"
    [(set_attr "type" "sseicvt")
     (set_attr "prefix" "maybe_vex")
+    (set (attr "nondfa_insn") (const_int 1))
     (set_attr "mode" "<MODE>")
     (set_attr "athlon_decode" "double,vector")
     (set_attr "amdfam10_decode" "double,double")
*************** (define_insn "x86_fnstcw_1"
*** 4632,4637 ****
--- 4644,4650 ----
    [(set (attr "length")
  	(symbol_ref "ix86_attr_length_address_default (insn) + 2"))
     (set_attr "mode" "HI")
+    (set (attr "nondfa_insn") (const_int 1))
     (set_attr "unit" "i387")
     (set_attr "bdver1_decode" "vector")])
  
*************** (define_insn "x86_fldcw_1"
*** 4642,4647 ****
--- 4655,4661 ----
    "fldcw\t%0"
    [(set (attr "length")
  	(symbol_ref "ix86_attr_length_address_default (insn) + 2"))
+    (set (attr "nondfa_insn") (const_int 1))
     (set_attr "mode" "HI")
     (set_attr "unit" "i387")
     (set_attr "athlon_decode" "vector")
*************** (define_insn "*float<SWI48x:mode><MODEF:
*** 5086,5091 ****
--- 5100,5106 ----
    "%vcvtsi2<MODEF:ssemodesuffix><SWI48x:rex64suffix>\t{%1, %d0|%d0, %1}"
    [(set_attr "type" "sseicvt")
     (set_attr "prefix" "maybe_vex")
+    (set (attr "nondfa_insn") (const_int 1))
     (set_attr "mode" "<MODEF:MODE>")
     (set (attr "prefix_rex")
       (if_then_else
*************** (define_insn "*bswap<mode>2_1"
*** 12213,12218 ****
--- 12228,12234 ----
    "bswap\t%0"
    [(set_attr "type" "bitmanip")
     (set_attr "modrm" "0")
+    (set (attr "nondfa_insn") (const_int 1))
     (set_attr "mode" "<MODE>")])
  
  (define_insn "*bswaphi_lowpart_1"
*************** (define_insn "*sqrt<mode>2_sse"
*** 13143,13148 ****
--- 13159,13165 ----
    "%vsqrt<ssemodesuffix>\t{%1, %d0|%d0, %1}"
    [(set_attr "type" "sse")
     (set_attr "atom_sse_attr" "sqrt")
+    (set (attr "nondfa_insn") (const_int 1))
     (set_attr "prefix" "maybe_vex")
     (set_attr "mode" "<MODE>")
     (set_attr "athlon_decode" "*")
*************** (define_insn "fxam<mode>2_i387"
*** 15209,15214 ****
--- 15226,15232 ----
    [(set_attr "type" "multi")
     (set_attr "length" "4")
     (set_attr "unit" "i387")
+    (set (attr "nondfa_insn") (const_int 1))
     (set_attr "mode" "<MODE>")])
  
  (define_insn_and_split "fxam<mode>2_i387_with_temp"
Index: gcc/config/i386/sse.md
===================================================================
*** gcc/config/i386/sse.md	(revision 175757)
--- gcc/config/i386/sse.md	(working copy)
*************** (define_insn "*mov<mode>_internal"
*** 266,271 ****
--- 266,272 ----
      }
  }
    [(set_attr "type" "sselog1,ssemov,ssemov")
+    (set (attr "nondfa_insn") (const_int 1))
     (set_attr "prefix" "maybe_vex")
     (set (attr "mode")
  	(cond [(ne (symbol_ref "TARGET_AVX") (const_int 0))
*************** (define_insn "*<sse2>_movdqu<avxsizesuff
*** 423,428 ****
--- 424,430 ----
       (const_string "*")
       (const_string "1")))
     (set_attr "prefix" "maybe_vex")
+    (set (attr "nondfa_insn") (const_int 1))
     (set_attr "mode" "<sseinsnmode>")])
  
  (define_insn "<sse3>_lddqu<avxsizesuffix>"
*************** (define_insn "*<plusminus_insn><mode>3"
*** 569,574 ****
--- 571,577 ----
     v<plusminus_mnemonic><ssemodesuffix>\t{%2, %1, %0|%0, %1, %2}"
    [(set_attr "isa" "noavx,avx")
     (set_attr "type" "sseadd")
+    (set (attr "nondfa_insn") (const_int 1))
     (set_attr "prefix" "orig,vex")
     (set_attr "mode" "<MODE>")])
  
*************** (define_insn "*mul<mode>3"
*** 608,613 ****
--- 611,617 ----
     vmul<ssemodesuffix>\t{%2, %1, %0|%0, %1, %2}"
    [(set_attr "isa" "noavx,avx")
     (set_attr "type" "ssemul")
+    (set (attr "nondfa_insn") (const_int 1))
     (set_attr "prefix" "orig,vex")
     (set_attr "mode" "<MODE>")])
  
*************** (define_insn "sse_storehps"
*** 3166,3171 ****
--- 3170,3176 ----
     %vmovhlps\t{%1, %d0|%d0, %1}
     %vmovlps\t{%H1, %d0|%d0, %H1}"
    [(set_attr "type" "ssemov")
+    (set (attr "nondfa_insn") (const_int 1))
     (set_attr "prefix" "maybe_vex")
     (set_attr "mode" "V2SF,V4SF,V2SF")])
  
*************** (define_insn "sse_loadhps"
*** 3205,3210 ****
--- 3210,3216 ----
     %vmovlps\t{%2, %H0|%H0, %2}"
    [(set_attr "isa" "noavx,avx,noavx,avx,*")
     (set_attr "type" "ssemov")
+    (set (attr "nondfa_insn") (const_int 1))
     (set_attr "prefix" "orig,vex,orig,vex,maybe_vex")
     (set_attr "mode" "V2SF,V2SF,V4SF,V4SF,V2SF")])
  
*************** (define_insn "sse_storelps"
*** 3219,3224 ****
--- 3225,3231 ----
     %vmovaps\t{%1, %0|%0, %1}
     %vmovlps\t{%1, %d0|%d0, %1}"
    [(set_attr "type" "ssemov")
+    (set (attr "nondfa_insn") (const_int 1))
     (set_attr "prefix" "maybe_vex")
     (set_attr "mode" "V2SF,V4SF,V2SF")])
  
*************** (define_insn "sse_loadlps"
*** 3258,3263 ****
--- 3265,3271 ----
     %vmovlps\t{%2, %0|%0, %2}"
    [(set_attr "isa" "noavx,avx,noavx,avx,*")
     (set_attr "type" "sselog,sselog,ssemov,ssemov,ssemov")
+    (set (attr "nondfa_insn") (const_int 1))
     (set_attr "length_immediate" "1,1,*,*,*")
     (set_attr "prefix" "orig,vex,orig,vex,maybe_vex")
     (set_attr "mode" "V4SF,V4SF,V2SF,V2SF,V2SF")])
*************** (define_insn "vec_set<mode>_0"
*** 3414,3419 ****
--- 3422,3428 ----
  	   (const_string "ssemov")))
     (set_attr "prefix_extra" "*,*,*,*,*,*,1,1,*,*,*")
     (set_attr "length_immediate" "*,*,*,*,*,*,1,1,*,*,*")
+    (set (attr "nondfa_insn") (const_int 1))
     (set_attr "prefix" "maybe_vex,maybe_vex,maybe_vex,orig,orig,vex,orig,vex,*,*,*")
     (set_attr "mode" "SF,<ssescalarmode>,SI,SF,SF,SF,TI,TI,*,*,*")])
  
*************** (define_insn "*vec_interleave_highv2df"
*** 3834,3839 ****
--- 3843,3849 ----
    [(set_attr "isa" "noavx,avx,*,noavx,avx,*")
    (set_attr "type" "sselog,sselog,sselog,ssemov,ssemov,ssemov")
     (set_attr "prefix_data16" "*,*,*,1,*,1")
+    (set (attr "nondfa_insn") (const_int 1))
     (set_attr "prefix" "orig,vex,maybe_vex,orig,vex,maybe_vex")
     (set_attr "mode" "V2DF,V2DF,V2DF,V1DF,V1DF,V1DF")])
  
*************** (define_insn "*vec_interleave_lowv2df"
*** 3934,3939 ****
--- 3944,3950 ----
     vmovhpd\t{%2, %1, %0|%0, %1, %2}
     %vmovlpd\t{%2, %H0|%H0, %2}"
    [(set_attr "isa" "noavx,avx,*,noavx,avx,*")
+    (set (attr "nondfa_insn") (const_int 1))
     (set_attr "type" "sselog,sselog,sselog,ssemov,ssemov,ssemov")
     (set_attr "prefix_data16" "*,*,*,1,*,1")
     (set_attr "prefix" "orig,vex,maybe_vex,orig,vex,maybe_vex")
*************** (define_insn "sse2_storehpd"
*** 4140,4145 ****
--- 4151,4157 ----
     #
     #"
    [(set_attr "isa" "*,noavx,avx,*,*,*")
+    (set (attr "nondfa_insn") (const_int 1))
     (set_attr "type" "ssemov,sselog1,sselog1,ssemov,fmov,imov")
     (set (attr "prefix_data16")
       (if_then_else
*************** (define_insn "sse2_storelpd"
*** 4189,4194 ****
--- 4201,4207 ----
     #"
    [(set_attr "type" "ssemov,ssemov,ssemov,fmov,imov")
     (set_attr "prefix_data16" "1,*,*,*,*")
+    (set (attr "nondfa_insn") (const_int 1))
     (set_attr "prefix" "maybe_vex")
     (set_attr "mode" "V1DF,DF,DF,DF,DF")])
  
*************** (define_insn "*vec_dupv2df"
*** 4408,4413 ****
--- 4421,4427 ----
    "TARGET_SSE2"
    "unpcklpd\t%0, %0"
    [(set_attr "type" "sselog1")
+    (set (attr "nondfa_insn") (const_int 1))
     (set_attr "mode" "V2DF")])
  
  (define_insn "*vec_concatv2df_sse3"
*************** (define_insn "*vec_concatv2df"
*** 4436,4441 ****
--- 4450,4456 ----
     movlhps\t{%2, %0|%0, %2}
     movhps\t{%2, %0|%0, %2}"
    [(set_attr "isa" "noavx,avx,noavx,avx,*,noavx,noavx")
+    (set (attr "nondfa_insn") (const_int 1))
     (set (attr "type")
       (if_then_else
         (eq_attr "alternative" "0,1")
*************** (define_insn "*<plusminus_insn><mode>3"
*** 4478,4483 ****
--- 4493,4499 ----
     vp<plusminus_mnemonic><ssemodesuffix>\t{%2, %1, %0|%0, %1, %2}"
    [(set_attr "isa" "noavx,avx")
     (set_attr "type" "sseiadd")
+    (set (attr "nondfa_insn") (const_int 1))
     (set_attr "prefix_data16" "1,*")
     (set_attr "prefix" "orig,vex")
     (set_attr "mode" "TI")])
*************** (define_insn "*mulv8hi3"
*** 4563,4568 ****
--- 4579,4585 ----
    [(set_attr "isa" "noavx,avx")
     (set_attr "type" "sseimul")
     (set_attr "prefix_data16" "1,*")
+    (set (attr "nondfa_insn") (const_int 1))
     (set_attr "prefix" "orig,vex")
     (set_attr "mode" "TI")])
  
*************** (define_insn "*sse2_umulv2siv2di3"
*** 4629,4634 ****
--- 4646,4652 ----
     pmuludq\t{%2, %0|%0, %2}
     vpmuludq\t{%2, %1, %0|%0, %1, %2}"
    [(set_attr "isa" "noavx,avx")
+    (set (attr "nondfa_insn") (const_int 1))
     (set_attr "type" "sseimul")
     (set_attr "prefix_data16" "1,*")
     (set_attr "prefix" "orig,vex")
*************** (define_insn "ashr<mode>3"
*** 5126,5131 ****
--- 5144,5150 ----
     vpsra<ssemodesuffix>\t{%2, %1, %0|%0, %1, %2}"
    [(set_attr "isa" "noavx,avx")
     (set_attr "type" "sseishft")
+    (set (attr "nondfa_insn") (const_int 1))
     (set (attr "length_immediate")
       (if_then_else (match_operand 2 "const_int_operand" "")
         (const_string "1")
*************** (define_insn "lshr<mode>3"
*** 5145,5150 ****
--- 5164,5170 ----
     vpsrl<ssemodesuffix>\t{%2, %1, %0|%0, %1, %2}"
    [(set_attr "isa" "noavx,avx")
     (set_attr "type" "sseishft")
+    (set (attr "nondfa_insn") (const_int 1))
     (set (attr "length_immediate")
       (if_then_else (match_operand 2 "const_int_operand" "")
         (const_string "1")
*************** (define_insn "ashl<mode>3"
*** 5164,5169 ****
--- 5184,5190 ----
     vpsll<ssemodesuffix>\t{%2, %1, %0|%0, %1, %2}"
    [(set_attr "isa" "noavx,avx")
     (set_attr "type" "sseishft")
+    (set (attr "nondfa_insn") (const_int 1))
     (set (attr "length_immediate")
       (if_then_else (match_operand 2 "const_int_operand" "")
         (const_string "1")
*************** (define_insn "sse2_lshrv1ti3"
*** 5242,5247 ****
--- 5263,5269 ----
    [(set_attr "isa" "noavx,avx")
     (set_attr "type" "sseishft")
     (set_attr "length_immediate" "1")
+    (set (attr "nondfa_insn") (const_int 1))
     (set_attr "atom_unit" "sishuf")
     (set_attr "prefix_data16" "1,*")
     (set_attr "prefix" "orig,vex")
*************** (define_insn "*andnottf3"
*** 5803,5808 ****
--- 5825,5831 ----
     vpandn\t{%2, %1, %0|%0, %1, %2}"
    [(set_attr "isa" "noavx,avx")
     (set_attr "type" "sselog")
+    (set (attr "nondfa_insn") (const_int 1))
     (set_attr "prefix_data16" "1,*")
     (set_attr "prefix" "orig,vex")
     (set_attr "mode" "TI")])
*************** (define_insn "*<code>tf3"
*** 5829,5834 ****
--- 5852,5858 ----
     (set_attr "type" "sselog")
     (set_attr "prefix_data16" "1,*")
     (set_attr "prefix" "orig,vex")
+    (set (attr "nondfa_insn") (const_int 1))
     (set_attr "mode" "TI")])
  
  ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
*************** (define_insn "vec_interleave_highv16qi"
*** 5920,5925 ****
--- 5944,5950 ----
     vpunpckhbw\t{%2, %1, %0|%0, %1, %2}"
    [(set_attr "isa" "noavx,avx")
     (set_attr "type" "sselog")
+    (set (attr "nondfa_insn") (const_int 1))
     (set_attr "prefix_data16" "1,*")
     (set_attr "prefix" "orig,vex")
     (set_attr "mode" "TI")])
*************** (define_insn "vec_interleave_lowv16qi"
*** 5944,5949 ****
--- 5969,5975 ----
     vpunpcklbw\t{%2, %1, %0|%0, %1, %2}"
    [(set_attr "isa" "noavx,avx")
     (set_attr "type" "sselog")
+    (set (attr "nondfa_insn") (const_int 1))
     (set_attr "prefix_data16" "1,*")
     (set_attr "prefix" "orig,vex")
     (set_attr "mode" "TI")])
*************** (define_insn "vec_interleave_lowv8hi"
*** 5984,5989 ****
--- 6010,6016 ----
     vpunpcklwd\t{%2, %1, %0|%0, %1, %2}"
    [(set_attr "isa" "noavx,avx")
     (set_attr "type" "sselog")
+    (set (attr "nondfa_insn") (const_int 1))
     (set_attr "prefix_data16" "1,*")
     (set_attr "prefix" "orig,vex")
     (set_attr "mode" "TI")])
*************** (define_insn "vec_interleave_lowv4si"
*** 6020,6025 ****
--- 6047,6053 ----
     vpunpckldq\t{%2, %1, %0|%0, %1, %2}"
    [(set_attr "isa" "noavx,avx")
     (set_attr "type" "sselog")
+    (set (attr "nondfa_insn") (const_int 1))
     (set_attr "prefix_data16" "1,*")
     (set_attr "prefix" "orig,vex")
     (set_attr "mode" "TI")])
*************** (define_insn "sse2_pshufd_1"
*** 6222,6227 ****
--- 6250,6256 ----
  }
    [(set_attr "type" "sselog1")
     (set_attr "prefix_data16" "1")
+    (set (attr "nondfa_insn") (const_int 1))
     (set_attr "prefix" "maybe_vex")
     (set_attr "length_immediate" "1")
     (set_attr "mode" "TI")])
*************** (define_insn "*vec_dupv4si"
*** 6479,6484 ****
--- 6508,6514 ----
     pshufd\t{$0, %1, %0|%0, %1, 0}
     shufps\t{$0, %0, %0|%0, %0, 0}"
    [(set_attr "type" "sselog1")
+    (set (attr "nondfa_insn") (const_int 1))
     (set_attr "length_immediate" "1")
     (set_attr "mode" "TI,V4SF")])
  
*************** (define_insn "*vec_dupv2di"
*** 6505,6510 ****
--- 6535,6541 ----
     punpcklqdq\t%0, %0
     movlhps\t%0, %0"
    [(set_attr "type" "sselog1,ssemov")
+    (set (attr "nondfa_insn") (const_int 1))
     (set_attr "mode" "TI,V4SF")])
  
  (define_insn "*vec_concatv2si_sse4_1"
*************** (define_insn "sse_ldmxcsr"
*** 6827,6832 ****
--- 6858,6864 ----
    "%vldmxcsr\t%0"
    [(set_attr "type" "sse")
     (set_attr "atom_sse_attr" "mxcsr")
+    (set (attr "nondfa_insn") (const_int 1))
     (set_attr "prefix" "maybe_vex")
     (set_attr "memory" "load")])
  
*************** (define_insn "sse_stmxcsr"
*** 6836,6841 ****
--- 6868,6874 ----
    "TARGET_SSE"
    "%vstmxcsr\t%0"
    [(set_attr "type" "sse")
+    (set (attr "nondfa_insn") (const_int 1))
     (set_attr "atom_sse_attr" "mxcsr")
     (set_attr "prefix" "maybe_vex")
     (set_attr "memory" "store")])
*************** (define_insn "*sse2_mfence"
*** 6883,6888 ****
--- 6916,6922 ----
    "TARGET_64BIT || TARGET_SSE2"
    "mfence"
    [(set_attr "type" "sse")
+    (set (attr "nondfa_insn") (const_int 1))
     (set_attr "length_address" "0")
     (set_attr "atom_sse_attr" "fence")
     (set_attr "memory" "unknown")])
*************** (define_insn "vec_dup<mode>"
*** 9783,9788 ****
--- 9817,9823 ----
     #"
    [(set_attr "type" "ssemov")
     (set_attr "prefix_extra" "1")
+    (set (attr "nondfa_insn") (const_int 1))
     (set_attr "prefix" "vex")
     (set_attr "mode" "V8SF")])
  

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: Fix PR 49014
  2011-07-01 14:50         ` Andrey Belevantsev
@ 2011-07-07 16:28           ` Vladimir Makarov
  2011-07-08  8:27             ` Andrey Belevantsev
  2011-07-07 17:13           ` Bernd Schmidt
  1 sibling, 1 reply; 10+ messages in thread
From: Vladimir Makarov @ 2011-07-07 16:28 UTC (permalink / raw)
  To: Andrey Belevantsev; +Cc: Bernd Schmidt, GCC Patches

On 07/01/2011 10:50 AM, Andrey Belevantsev wrote:
> On 26.05.2011 17:32, Andrey Belevantsev wrote:
>> On 25.05.2011 19:31, Bernd Schmidt wrote:
>>> On 05/25/2011 03:29 PM, Andrey Belevantsev wrote:
>>>> I think the hook is a better idea than the attribute because nobody 
>>>> will
>>>> care to mark all offending insns with an attribute.
>>>
>>> I don't know. IIRC when I looked at sh or whatever the broken port was,
>>> it was only two insns - there would still be some value in being 
>>> able to
>>> assert that all other insns have a reservation.
>> OK, I will take a look on x86-64 and will get back with more 
>> information.
>>
>> Andrey
> So, I have made an attempt to bootstrap on x86-64 with the extra 
> assert in selective scheduling that assumes the DFA state always 
> changes when issuing a recog_memoized >=0 insn (patch attached).  
> Indeed, there are just a few general insns that don't have proper 
> reservations.  However, it was a surprise to me to see that almost any 
> insn with SSE registers fails this assert and thus does not get 
> properly scheduled.
>
> Overall, the work on fixing those seems doable, it took just a day to 
> get the compiler bootstrapped (of course, the testsuite may bring much 
> more issues).  So, if there is an agreement on marking a few offending 
> insns with the new attribute, we can proceed with the help of somebody 
> from the x86 land on fixing those and researching for other targets.
>
The changes in sel-sched.c is ok for me.  i386.md changes look ok for me 
too but you should ask a x86 maintainer to get an approval for the change.

I think you should describe the attribute in the documentation because 
it is common for all targets.

I can not approve common.opt changes because it makes selective 
scheduler is default for the 2nd insn scheduling for all targets.  Such 
change should be justified by thorough testing and benchmarking 
(compilation speed, code size, performance improvements) on several 
platforms (at least on major ones).

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: Fix PR 49014
  2011-07-07 16:28           ` Vladimir Makarov
@ 2011-07-08  8:27             ` Andrey Belevantsev
  2011-07-08 15:38               ` Vladimir Makarov
  0 siblings, 1 reply; 10+ messages in thread
From: Andrey Belevantsev @ 2011-07-08  8:27 UTC (permalink / raw)
  To: Vladimir Makarov; +Cc: Bernd Schmidt, GCC Patches

On 07.07.2011 20:18, Vladimir Makarov wrote:
> On 07/01/2011 10:50 AM, Andrey Belevantsev wrote:
>> On 26.05.2011 17:32, Andrey Belevantsev wrote:
>>> On 25.05.2011 19:31, Bernd Schmidt wrote:
>>>> On 05/25/2011 03:29 PM, Andrey Belevantsev wrote:
>>>>> I think the hook is a better idea than the attribute because nobody will
>>>>> care to mark all offending insns with an attribute.
>>>>
>>>> I don't know. IIRC when I looked at sh or whatever the broken port was,
>>>> it was only two insns - there would still be some value in being able to
>>>> assert that all other insns have a reservation.
>>> OK, I will take a look on x86-64 and will get back with more information.
>>>
>>> Andrey
>> So, I have made an attempt to bootstrap on x86-64 with the extra assert
>> in selective scheduling that assumes the DFA state always changes when
>> issuing a recog_memoized >=0 insn (patch attached). Indeed, there are
>> just a few general insns that don't have proper reservations. However, it
>> was a surprise to me to see that almost any insn with SSE registers fails
>> this assert and thus does not get properly scheduled.
>>
>> Overall, the work on fixing those seems doable, it took just a day to get
>> the compiler bootstrapped (of course, the testsuite may bring much more
>> issues). So, if there is an agreement on marking a few offending insns
>> with the new attribute, we can proceed with the help of somebody from the
>> x86 land on fixing those and researching for other targets.
>>
> The changes in sel-sched.c is ok for me. i386.md changes look ok for me too
> but you should ask a x86 maintainer to get an approval for the change.
>
> I think you should describe the attribute in the documentation because it
> is common for all targets.
>
> I can not approve common.opt changes because it makes selective scheduler
> is default for the 2nd insn scheduling for all targets. Such change should
> be justified by thorough testing and benchmarking (compilation speed, code
> size, performance improvements) on several platforms (at least on major ones).
I didn't intend to enable sel-sched for all targets, the patch was just an 
RFC to see whether there is an agreement about usefulness of such 
attribute, and the common.opt change was to show how I tested the patch.  I 
am sorry for not making it clear in the mail.

I am planning to check Bernd's thought about whether I selected the right 
-mcpu switch for testing, as I was under impression that nowadays this 
should be autodetected by configure.  I will also modify the attribute as 
suggested.  Then we can discuss further.  I am going to leave on vacation 
soon though so I don't know when exactly I can proceed with this.

Thanks!

Andrey

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: Fix PR 49014
  2011-07-08  8:27             ` Andrey Belevantsev
@ 2011-07-08 15:38               ` Vladimir Makarov
  0 siblings, 0 replies; 10+ messages in thread
From: Vladimir Makarov @ 2011-07-08 15:38 UTC (permalink / raw)
  To: Andrey Belevantsev; +Cc: Bernd Schmidt, GCC Patches

On 11-07-08 3:25 AM, Andrey Belevantsev wrote:
> On 07.07.2011 20:18, Vladimir Makarov wrote:
>>
>> The changes in sel-sched.c is ok for me. i386.md changes look ok for 
>> me too
>> but you should ask a x86 maintainer to get an approval for the change.
>>
>> I think you should describe the attribute in the documentation 
>> because it
>> is common for all targets.
>>
>> I can not approve common.opt changes because it makes selective 
>> scheduler
>> is default for the 2nd insn scheduling for all targets. Such change 
>> should
>> be justified by thorough testing and benchmarking (compilation speed, 
>> code
>> size, performance improvements) on several platforms (at least on 
>> major ones).
> I didn't intend to enable sel-sched for all targets, the patch was 
> just an RFC to see whether there is an agreement about usefulness of 
> such attribute, and the common.opt change was to show how I tested the 
> patch.  I am sorry for not making it clear in the mail.
>
Sorry, for my misunderstanding.  The patch itself with some work could 
be submitted because the check is in the selective scheduling and it is 
used as default just for few targets.

If the check were in haifa-scheduler, we would have a lot of troubles 
and broken targets. Many targets have a lot of subtargets and I am sure 
a lot of their descriptions are not full.  To be honest, I have no idea 
how to solve the problem of absence of some insn dfa descriptions with a 
small pain.  In any case, a big target maintainers involvement will be 
required.

I guess, if we did the check optional, it could help.  The target 
maintainers could switch on the check and fix the insn description 
absence if they want and when they want.
> I am planning to check Bernd's thought about whether I selected the 
> right -mcpu switch for testing, as I was under impression that 
> nowadays this should be autodetected by configure.  I will also modify 
> the attribute as suggested.  Then we can discuss further.  I am going 
> to leave on vacation soon though so I don't know when exactly I can 
> proceed with this.
>
Ok. Have a nice vacation.
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: Fix PR 49014
  2011-07-01 14:50         ` Andrey Belevantsev
  2011-07-07 16:28           ` Vladimir Makarov
@ 2011-07-07 17:13           ` Bernd Schmidt
  1 sibling, 0 replies; 10+ messages in thread
From: Bernd Schmidt @ 2011-07-07 17:13 UTC (permalink / raw)
  To: Andrey Belevantsev; +Cc: GCC Patches, Vladimir N. Makarov

On 07/01/11 16:50, Andrey Belevantsev wrote:
> On 26.05.2011 17:32, Andrey Belevantsev wrote:
>> On 25.05.2011 19:31, Bernd Schmidt wrote:
>>> On 05/25/2011 03:29 PM, Andrey Belevantsev wrote:
>>>> I think the hook is a better idea than the attribute because nobody
>>>> will
>>>> care to mark all offending insns with an attribute.
>>>
>>> I don't know. IIRC when I looked at sh or whatever the broken port was,
>>> it was only two insns - there would still be some value in being able to
>>> assert that all other insns have a reservation.
>> OK, I will take a look on x86-64 and will get back with more information.
>>
>> Andrey
> So, I have made an attempt to bootstrap on x86-64 with the extra assert
> in selective scheduling that assumes the DFA state always changes when
> issuing a recog_memoized >=0 insn (patch attached).  Indeed, there are
> just a few general insns that don't have proper reservations.  However,
> it was a surprise to me to see that almost any insn with SSE registers
> fails this assert and thus does not get properly scheduled.

Probably because it's picking a scheduling description for an old CPU?
With "-mcpu=pentium" probably none of the newer patterns has a reservation.

That may scupper any plans to use this attribute on i386.

> Overall, the work on fixing those seems doable, it took just a day to
> get the compiler bootstrapped (of course, the testsuite may bring much
> more issues).  So, if there is an agreement on marking a few offending
> insns with the new attribute, we can proceed with the help of somebody
> from the x86 land on fixing those and researching for other targets.

+    (set (attr "nondfa_insn") (if_then_else (eq_attr "alternative"
"3,4,5,6") (const_int 1) (const_int 0)))

I think this shouldn't use (const_int x); you want to be able to write
 (set_attr "nondfa_insn" "0,0,0,1,1,1,1")


Bernd

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2011-07-08 15:16 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-05-25  9:45 Fix PR 49014 Andrey Belevantsev
2011-05-25 14:57 ` Bernd Schmidt
2011-05-25 16:42   ` Andrey Belevantsev
2011-05-25 16:49     ` Bernd Schmidt
2011-05-26 13:53       ` Andrey Belevantsev
2011-07-01 14:50         ` Andrey Belevantsev
2011-07-07 16:28           ` Vladimir Makarov
2011-07-08  8:27             ` Andrey Belevantsev
2011-07-08 15:38               ` Vladimir Makarov
2011-07-07 17:13           ` Bernd Schmidt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).