| Message ID | ceb839ae-0101-11d2-d5ce-b402c7dcfe51@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
> [This is a repost of a patch previously posted on 3/29/2017. > Eric, I hope you might consider that this falls within your scope > of maintenance. Thanks.] My viewpoint is that it's better to keep the assertions and fix the back-end instead, which looks rather straightforward.
On Thu, Apr 06, 2017 at 10:05:54PM +0200, Eric Botcazou wrote: > > [This is a repost of a patch previously posted on 3/29/2017. > > Eric, I hope you might consider that this falls within your scope > > of maintenance. Thanks.] > > My viewpoint is that it's better to keep the assertions and fix the back-end > instead, which looks rather straightforward. The only straightforward way I see is to use a rs6000_store_data_bypass_p instead, which would be doing the same thing. :-( [ It of course is easy to workaround the specific problem in the testcase, but that does not solve any of the underlying problems. ] Segher
> The only straightforward way I see is to use a rs6000_store_data_bypass_p > instead, which would be doing the same thing. :-( Why not just change the type of the blockage instruction as you suggested?
On Fri, Apr 07, 2017 at 08:54:01AM +0200, Eric Botcazou wrote: > > The only straightforward way I see is to use a rs6000_store_data_bypass_p > > instead, which would be doing the same thing. :-( > > Why not just change the type of the blockage instruction as you suggested? That works for this case, sure, but it won't solve the underlying problem. The default instruction type for rs6000 is "integer". Many instructions have this type: some bswap, register moves, load address/immediate, mtvrsave, nop, and many things that are split; and that is just those that have type "*" (so are easy to search for), not all those that do no explicitly have a type attribute (including blockage). Now, the power6 scheduler defines a bypass from power6-integer (which is types "integer" as well as "add" and "logical") to stores. And then store_data_bypass_p ICEs because not all "integer" insns are a SET or a PARALLEL of SETs. So, changing the type involves changing the default to something else, changing all pipeline descriptions to do the same with that type as it does with "integer", checking all patterns that are type "integer" to see if they should be that new type or not. Or we could just change "blockage" and wait for the next bug report. Alternatively, we can arrange for the bypass functions to not ICE. We can do that specific to these rs6000 pipeline descriptions, by having our own version of store_data_bypass_p; or we can make that function work for all insns (its definition works fine for insn pairs where not both the producer and consumer are SETs). That's what Kelvin's patch does. What is the value in ICEing here? Segher
> Or we could just change "blockage" and wait for the next bug report. That's my suggestion, yes. > Alternatively, we can arrange for the bypass functions to not ICE. We > can do that specific to these rs6000 pipeline descriptions, by having > our own version of store_data_bypass_p; or we can make that function > work for all insns (its definition works fine for insn pairs where > not both the producer and consumer are SETs). That's what Kelvin's > patch does. What is the value in ICEing here? Telling the back-end writer that something may be wrong somewhere instead of silently accepting nonsense? How long have all the assertions been there?
On Fri, Apr 07, 2017 at 10:39:03AM +0200, Eric Botcazou wrote: > > Or we could just change "blockage" and wait for the next bug report. > > That's my suggestion, yes. > > > Alternatively, we can arrange for the bypass functions to not ICE. We > > can do that specific to these rs6000 pipeline descriptions, by having > > our own version of store_data_bypass_p; or we can make that function > > work for all insns (its definition works fine for insn pairs where > > not both the producer and consumer are SETs). That's what Kelvin's > > patch does. What is the value in ICEing here? > > Telling the back-end writer that something may be wrong somewhere instead of > silently accepting nonsense? Why is it nonsense? The predicate gives the answer to the question "given these insns A and B, does A feed data that B stores in memory". That is a perfectly valid question to ask of any two insns. > How long have all the assertions been there? There are workarounds to this problem as well: mips_store_data_bypass_p, added in 2006. mep_store_data_bypass_p, added in 2009 (the port has been removed since then, of course). Segher
Segher Boessenkool <segher@kernel.crashing.org> writes: > On Fri, Apr 07, 2017 at 10:39:03AM +0200, Eric Botcazou wrote: >> > Or we could just change "blockage" and wait for the next bug report. >> >> That's my suggestion, yes. >> >> > Alternatively, we can arrange for the bypass functions to not ICE. We >> > can do that specific to these rs6000 pipeline descriptions, by having >> > our own version of store_data_bypass_p; or we can make that function >> > work for all insns (its definition works fine for insn pairs where >> > not both the producer and consumer are SETs). That's what Kelvin's >> > patch does. What is the value in ICEing here? >> >> Telling the back-end writer that something may be wrong somewhere instead of >> silently accepting nonsense? > > Why is it nonsense? The predicate gives the answer to the question > "given these insns A and B, does A feed data that B stores in memory". > That is a perfectly valid question to ask of any two insns. Agreed FWIW, but for: @@ -3701,7 +3704,8 @@ store_data_bypass_p (rtx_insn *out_insn, rtx_insn if (GET_CODE (out_exp) == CLOBBER) continue; - gcc_assert (GET_CODE (out_exp) == SET); + if (GET_CODE (out_exp) != SET) + return false; if (reg_mentioned_p (SET_DEST (out_exp), SET_DEST (in_set))) return false; how about instead changing the CLOBBER check so that we continue when it isn't a SET? That would allow things like UNSPECs and USEs as well. Thanks, Richard
On Mon, Apr 10, 2017 at 06:38:07PM +0100, Richard Sandiford wrote: > Segher Boessenkool <segher@kernel.crashing.org> writes: > > On Fri, Apr 07, 2017 at 10:39:03AM +0200, Eric Botcazou wrote: > >> > Or we could just change "blockage" and wait for the next bug report. > >> > >> That's my suggestion, yes. > >> > >> > Alternatively, we can arrange for the bypass functions to not ICE. We > >> > can do that specific to these rs6000 pipeline descriptions, by having > >> > our own version of store_data_bypass_p; or we can make that function > >> > work for all insns (its definition works fine for insn pairs where > >> > not both the producer and consumer are SETs). That's what Kelvin's > >> > patch does. What is the value in ICEing here? > >> > >> Telling the back-end writer that something may be wrong somewhere instead of > >> silently accepting nonsense? > > > > Why is it nonsense? The predicate gives the answer to the question > > "given these insns A and B, does A feed data that B stores in memory". > > That is a perfectly valid question to ask of any two insns. > > Agreed FWIW, but for: > > @@ -3701,7 +3704,8 @@ store_data_bypass_p (rtx_insn *out_insn, rtx_insn > if (GET_CODE (out_exp) == CLOBBER) > continue; > > - gcc_assert (GET_CODE (out_exp) == SET); > + if (GET_CODE (out_exp) != SET) > + return false; > > if (reg_mentioned_p (SET_DEST (out_exp), SET_DEST (in_set))) > return false; > > how about instead changing the CLOBBER check so that we continue > when it isn't a SET? That would allow things like UNSPECs and > USEs as well. Yeah that sounds good. Kelvin, could you try that please? Segher
[Sorry for the long delay] > Why is it nonsense? The predicate gives the answer to the question > "given these insns A and B, does A feed data that B stores in memory". > That is a perfectly valid question to ask of any two insns. I disagree, for example it's nonsensical to send it a blockage insn. > There are workarounds to this problem as well: mips_store_data_bypass_p, > added in 2006. mep_store_data_bypass_p, added in 2009 (the port has > been removed since then, of course). I see, no strong opinion then, but individual back-ends should be preferably fixed if this is easily doable instead of changing the middle-end, which may affect the other ~50 back-ends.
On Tue, Apr 18, 2017 at 10:01:02PM +0200, Eric Botcazou wrote: > > There are workarounds to this problem as well: mips_store_data_bypass_p, > > added in 2006. mep_store_data_bypass_p, added in 2009 (the port has > > been removed since then, of course). > > I see, no strong opinion then, but individual back-ends should be preferably > fixed if this is easily doable instead of changing the middle-end, which may > affect the other ~50 back-ends. Okay, for GCC 7 we'll change the backend. Maybe for GCC 8 we can revisit this. Thanks, Segher
Index: gcc/recog.c =================================================================== --- gcc/recog.c (revision 246469) +++ gcc/recog.c (working copy) @@ -3663,9 +3663,12 @@ peephole2_optimize (void) /* Common predicates for use with define_bypass. */ -/* True if the dependency between OUT_INSN and IN_INSN is on the store - data not the address operand(s) of the store. IN_INSN and OUT_INSN - must be either a single_set or a PARALLEL with SETs inside. */ +/* Returns true if the dependency between OUT_INSN and IN_INSN is on + the stored data, false if there is no dependency. Note that a + consumer instruction that loads only the address (rather than the + value) stored by a producer instruction does not represent a + dependency. If IN_INSN or OUT_INSN are not a single_set or a + PARALLEL with SETs inside, this function returns false. */ int store_data_bypass_p (rtx_insn *out_insn, rtx_insn *in_insn) @@ -3701,7 +3704,8 @@ store_data_bypass_p (rtx_insn *out_insn, rtx_insn if (GET_CODE (out_exp) == CLOBBER) continue; - gcc_assert (GET_CODE (out_exp) == SET); + if (GET_CODE (out_exp) != SET) + return false; if (reg_mentioned_p (SET_DEST (out_exp), SET_DEST (in_set))) return false; @@ -3711,7 +3715,8 @@ store_data_bypass_p (rtx_insn *out_insn, rtx_insn else { in_pat = PATTERN (in_insn); - gcc_assert (GET_CODE (in_pat) == PARALLEL); + if (GET_CODE (in_pat) != PARALLEL) + return false; for (i = 0; i < XVECLEN (in_pat, 0); i++) { @@ -3720,7 +3725,8 @@ store_data_bypass_p (rtx_insn *out_insn, rtx_insn if (GET_CODE (in_exp) == CLOBBER) continue; - gcc_assert (GET_CODE (in_exp) == SET); + if (GET_CODE (in_exp) != SET) + return false; if (!MEM_P (SET_DEST (in_exp))) return false; @@ -3734,7 +3740,8 @@ store_data_bypass_p (rtx_insn *out_insn, rtx_insn else { out_pat = PATTERN (out_insn); - gcc_assert (GET_CODE (out_pat) == PARALLEL); + if (GET_CODE (out_pat) != PARALLEL) + return false; for (j = 0; j < XVECLEN (out_pat, 0); j++) { @@ -3743,7 +3750,8 @@ store_data_bypass_p (rtx_insn *out_insn, rtx_insn if (GET_CODE (out_exp) == CLOBBER) continue; - gcc_assert (GET_CODE (out_exp) == SET); + if (GET_CODE (out_exp) != SET) + return false; if (reg_mentioned_p (SET_DEST (out_exp), SET_DEST (in_exp))) return false; Index: gcc/testsuite/gcc.target/powerpc/pr80101-1.c =================================================================== --- gcc/testsuite/gcc.target/powerpc/pr80101-1.c (revision 0) +++ gcc/testsuite/gcc.target/powerpc/pr80101-1.c (working copy) @@ -0,0 +1,22 @@ +/* { dg-do compile { target { powerpc*-*-* } } } */ +/* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { "-mcpu=power6" } } */ +/* { dg-require-effective-target dfp_hw } */ +/* { dg-options "-mcpu=power6 -mno-sched-epilog -Ofast" } */ + +/* Prior to resolving PR 80101, this test case resulted in an internal + compiler error. The role of this test program is to assure that + dejagnu's "test for excess errors" does not find any. */ + +int b; + +void e (); + +int c () +{ + struct + { + int a[b]; + } d; + if (d.a[0]) + e (); +}