Re: [PATCH v3,rs6000] PR80101: Fix ICE in store_data_bypass_p

[Segher Boessenkool]

Hi Kelvin,

On Fri, Apr 21, 2017 at 10:01:05AM -0600, Kelvin Nilsen wrote:
> A new rs6000_store_data_bypass_p function has been introduced and all
> calls to store_data_bypass_p from within the rs6000 back end have been
> replaced with calls to rs6000_store_data_bypass_p.  This new function
> scans its arguments for patterns that are known to cause assertion
> errors in store_data_bypass_p and returns false if any of those
> patterns are encountered.  Otherwise, rs6000_store_data_bypass_p simply
> returns the result produced when passing its arguments to a call of
> store_data_bypass_p.

> 2017-04-20  Kelvin Nilsen  
> 
>   PR target/80101
>   * config/rs6000/power6.md: Replace store_data_bypass_p calls with
>   rs6000_store_data_bypass_p in seven define_bypass directives and
>   in several comments.

Interesting that this is the only scheduling description where we use
this...  Do power8 (etc.) really not need it?

>   * config/rs6000/rs6000-protos.h: Add prototype for
>   rs6000_store_data_bypass_p function.
>   * config/rs6000/rs6000.c (rs6000_store_data_bypass_p): New
>   function implements slightly different (rs6000-specific) semantics
>   than store_data_bypass_p, returning false rather than aborting
>   with assertion error when arguments do not satisfy the
>   requirements of store data bypass.
>   (rs6000_adjust_cost): Replace six calls of store_data_bypass_p with
>   rs6000_store_data_bypass_p.

The patch is fine for trunk.  Thanks,


Segher

[PATCH v3,rs6000] PR80101: Fix ICE in store_data_bypass_p

[Kelvin Nilsen]

This problem reports an assertion error when certain rtl expressions
which are not eligible as producers or consumers of a store bypass
optimization are passed as arguments to the store_data_bypass_p
function.  Since the problem surfaced with tests targeting the rs6000
architecture, the proposed patch is integrated within the rs6000 back
end.  

A new rs6000_store_data_bypass_p function has been introduced and all
calls to store_data_bypass_p from within the rs6000 back end have been
replaced with calls to rs6000_store_data_bypass_p.  This new function
scans its arguments for patterns that are known to cause assertion
errors in store_data_bypass_p and returns false if any of those
patterns are encountered.  Otherwise, rs6000_store_data_bypass_p simply
returns the result produced when passing its arguments to a call of
store_data_bypass_p.

Thank you for feedback and guidance from Eric Botcazou, Segher
Boessenkool, Richard Sandiford, and Pat Haugen which was offered in
response to my first two patch submissions and an RFC post on this
topic.  With all of your help, I now have a much better understanding
of the intended role of store_data_bypass_p.

The patch has been boostrapped without regressions on
powerpc64le-unknown-linux-gnu.  Is this ok for the trunk?

gcc/testsuite/ChangeLog:

2017-04-20  Kelvin Nilsen  

PR target/80101
* gcc.target/powerpc/pr80101-1.c: New test.


gcc/ChangeLog:

2017-04-20  Kelvin Nilsen  

PR target/80101
* config/rs6000/power6.md: Replace store_data_bypass_p calls with
rs6000_store_data_bypass_p in seven define_bypass directives and
in several comments.
* config/rs6000/rs6000-protos.h: Add prototype for
rs6000_store_data_bypass_p function.
* config/rs6000/rs6000.c (rs6000_store_data_bypass_p): New
function implements slightly different (rs6000-specific) semantics
than store_data_bypass_p, returning false rather than aborting
with assertion error when arguments do not satisfy the
requirements of store data bypass.
(rs6000_adjust_cost): Replace six calls of store_data_bypass_p with
rs6000_store_data_bypass_p.

Index: gcc/config/rs6000/power6.md
===
--- gcc/config/rs6000/power6.md (revision 246469)
+++ gcc/config/rs6000/power6.md (working copy)
@@ -108,7 +108,7 @@
   power6-store-update-indexed,\
   power6-fpstore,\
   power6-fpstore-update"
-  "store_data_bypass_p")
+  "rs6000_store_data_bypass_p")
 
 (define_insn_reservation "power6-load-ext" 4 ; fx
   (and (eq_attr "type" "load")
@@ -128,7 +128,7 @@
   power6-store-update-indexed,\
   power6-fpstore,\
   power6-fpstore-update"
-  "store_data_bypass_p")
+  "rs6000_store_data_bypass_p")
 
 (define_insn_reservation "power6-load-update" 2 ; fx
   (and (eq_attr "type" "load")
@@ -276,7 +276,7 @@
   power6-store-update-indexed,\
   power6-fpstore,\
   power6-fpstore-update"
-  "store_data_bypass_p")
+  "rs6000_store_data_bypass_p")
 
 (define_insn_reservation "power6-cntlz" 2
   (and (eq_attr "type" "cntlz")
@@ -289,7 +289,7 @@
   power6-store-update-indexed,\
   power6-fpstore,\
   power6-fpstore-update"
-  "store_data_bypass_p")
+  "rs6000_store_data_bypass_p")
 
 (define_insn_reservation "power6-var-rotate" 4
   (and (eq_attr "type" "shift")
@@ -355,7 +355,7 @@
   power6-store-update-indexed,\
   power6-fpstore,\
   power6-fpstore-update"
-  "store_data_bypass_p")
+  "rs6000_store_data_bypass_p")
 
 (define_insn_reservation "power6-delayed-compare" 2 ; N/A
   (and (eq_attr "type" "shift")
@@ -420,7 +420,7 @@
   power6-store-update-indexed,\
   power6-fpstore,\
   power6-fpstore-update"
-  "store_data_bypass_p")
+  "rs6000_store_data_bypass_p")
 
 (define_insn_reservation "power6-idiv" 44
   (and (eq_attr "type" "div")
@@ -436,7 +436,7 @@
 ;  power6-store-update-indexed,\
 ;  power6-fpstore,\
 ;  power6-fpstore-update"
-;  "store_data_bypass_p")
+;  "rs6000_store_data_bypass_p")
 
 (define_insn_reservation "power6-ldiv" 56
   (and (eq_attr "type" "div")
@@ -452,7 +452,7 @@
 ;  power6-store-update-indexed,\
 ;  power6-fpstore,\
 ;  power6-fpstore-update"
-;  "store_data_bypass_p")
+;  "rs6000_store_data_bypass_p")
 
 (define_insn_reservation "power6-mtjmpr" 2
   (and (eq_attr "type" "mtjmpr,mfjmpr")
@@ -510,7 +510,7 @@
 
 (define_bypass 1 "power6-fp"
  "power6-fpstore,power6-fpstore-update"
-  "store_data_bypass_p")
+  "rs6000_store_data_bypass_p")
 
 (define_insn_reservation "power6-fpcompare" 8
   (and (eq_attr