PR79066, non-PIC code generated for powerpc glibc with -fpic

PR79066, non-PIC code generated for powerpc glibc with -fpic

[Alan Modra]

Bootstrapped and regression tested powerpc64-linux biarch.  elf_high
has said "Elf specific ways of loading addresses for non-PIC code"
                                                     ^^^^^^^
right from the inital V4 support in 1995.

OK for mainline?

	PR target/79066
	* config/rs6000/rs6000.md (elf_high, elf_low): Disable when pic.
testsuite/
	* gcc.target/powerpc/pr79066.c: New.

diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
index 80c10a7..98209fa 100644
--- a/gcc/config/rs6000/rs6000.md
+++ b/gcc/config/rs6000/rs6000.md
@@ -10597,14 +10597,14 @@ (define_insn_and_split "*tocref<mode>"
 (define_insn "elf_high"
   [(set (match_operand:SI 0 "gpc_reg_operand" "=b*r")
 	(high:SI (match_operand 1 "" "")))]
-  "TARGET_ELF && ! TARGET_64BIT"
+  "TARGET_ELF && !TARGET_64BIT && !flag_pic"
   "lis %0,%1@ha")
 
 (define_insn "elf_low"
   [(set (match_operand:SI 0 "gpc_reg_operand" "=r")
 	(lo_sum:SI (match_operand:SI 1 "gpc_reg_operand" "b")
 		   (match_operand 2 "" "")))]
-   "TARGET_ELF && ! TARGET_64BIT"
+   "TARGET_ELF && !TARGET_64BIT && !flag_pic"
    "la %0,%2@l(%1)")
 \f
 ;; Call and call_value insns
diff --git a/gcc/testsuite/gcc.target/powerpc/pr79066.c b/gcc/testsuite/gcc.target/powerpc/pr79066.c
new file mode 100644
index 0000000..86b2014
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr79066.c
@@ -0,0 +1,14 @@
+/* { dg-do compile { target { fpic && ilp32 } } } */
+/* { dg-options "-O2 -fpic" } */
+/* { dg-final { scan-assembler-not "lis.*@ha" } } */
+
+union U { double x; int i[2]; };
+
+double
+foo (double x)
+{
+  union U v;
+  v.i[0] = 0x7ff00000;
+  v.i[1] = 0;
+  return x / v.x;
+}

-- 
Alan Modra
Australia Development Lab, IBM

Re: PR79066, non-PIC code generated for powerpc glibc with -fpic

[Segher Boessenkool]

On Fri, Jan 13, 2017 at 10:10:12PM +1030, Alan Modra wrote:
> Bootstrapped and regression tested powerpc64-linux biarch.  elf_high
> has said "Elf specific ways of loading addresses for non-PIC code"
>                                                      ^^^^^^^
> right from the inital V4 support in 1995.
> 
> OK for mainline?

Have you checked if/what this changes for some bigger code?

Okay for trunk if there is nothing unexpected.  Thanks!

Vladimir: Why does LRA attempt to manually construct high/lo_sum at all?
The next thing it tries is using lra_emit_move; this will also do it (on
all targets I checked), but only after appropriate (target-specific) checks.

It is way too late in stage 3 to attempt to change this now, of course ;-)


Segher


> 	PR target/79066
> 	* config/rs6000/rs6000.md (elf_high, elf_low): Disable when pic.
> testsuite/
> 	* gcc.target/powerpc/pr79066.c: New.

Re: PR79066, non-PIC code generated for powerpc glibc with -fpic

[Alan Modra]

On Sat, Jan 14, 2017 at 03:28:51AM -0600, Segher Boessenkool wrote:
> On Fri, Jan 13, 2017 at 10:10:12PM +1030, Alan Modra wrote:
> > Bootstrapped and regression tested powerpc64-linux biarch.  elf_high
> > has said "Elf specific ways of loading addresses for non-PIC code"
> >                                                      ^^^^^^^
> > right from the inital V4 support in 1995.
> > 
> > OK for mainline?
> 
> Have you checked if/what this changes for some bigger code?

Well, the bootstrap was all langs (minus ada due to not having ada
installed) and ppc32 multilibs were built.  Plus the testsuite run
with RUNTESTFLAGS="--target_board=unix'{-m32,-m64}{-mlra,-mno-lra}'"

I would have bootstrapped -m32 except the machine I used lacked 32-bit
gmp, mpfr, mpc and I was lazy.

> Okay for trunk if there is nothing unexpected.  Thanks!

I guess I should at least build glibc.

-- 
Alan Modra
Australia Development Lab, IBM

Re: PR79066, non-PIC code generated for powerpc glibc with -fpic

[Jakub Jelinek]

On Sun, Jan 15, 2017 at 12:08:39AM +1030, Alan Modra wrote:
> On Sat, Jan 14, 2017 at 03:28:51AM -0600, Segher Boessenkool wrote:
> > On Fri, Jan 13, 2017 at 10:10:12PM +1030, Alan Modra wrote:
> > > Bootstrapped and regression tested powerpc64-linux biarch.  elf_high
> > > has said "Elf specific ways of loading addresses for non-PIC code"
> > >                                                      ^^^^^^^
> > > right from the inital V4 support in 1995.
> > > 
> > > OK for mainline?
> > 
> > Have you checked if/what this changes for some bigger code?
> 
> Well, the bootstrap was all langs (minus ada due to not having ada
> installed) and ppc32 multilibs were built.  Plus the testsuite run
> with RUNTESTFLAGS="--target_board=unix'{-m32,-m64}{-mlra,-mno-lra}'"
> 
> I would have bootstrapped -m32 except the machine I used lacked 32-bit
> gmp, mpfr, mpc and I was lazy.
> 
> > Okay for trunk if there is nothing unexpected.  Thanks!
> 
> I guess I should at least build glibc.

See Uros' comment about the INSN_CODE (insn) = insn_code_number;.
Also, I'm worried about it for another reason, after the
if (!targetm.legitimate_combined_insn (insn))
call the PATTERN is reverted to something different, so keeping INSN_CODE
equal to insn_code_number (which we sometimes even change to -1)
looks wrong, if it is the right thing to do it for the
legitimate_combined_insn call, it should be reverted afterwards when the
PATTERN changes again.
Or, perhaps instead of setting INSN_CODE, pass insn_code_number to the
target hook as another argument?

	Jakub

Re: Avoid PR72749 by not using unspecs

[Alan Modra]

On Sat, Jan 14, 2017 at 02:46:11PM +0100, Jakub Jelinek wrote:
> See Uros' comment about the INSN_CODE (insn) = insn_code_number;.

Later email retracted the one about a crash.

> Also, I'm worried about it for another reason, after the
> if (!targetm.legitimate_combined_insn (insn))
> call the PATTERN is reverted to something different, so keeping INSN_CODE
> equal to insn_code_number (which we sometimes even change to -1)
> looks wrong, if it is the right thing to do it for the
> legitimate_combined_insn call, it should be reverted afterwards when the
> PATTERN changes again.

It is reverted afterwards.

      PATTERN (insn) = old_pat;
      REG_NOTES (insn) = old_notes;
      INSN_CODE (insn) = old_icode;

-- 
Alan Modra
Australia Development Lab, IBM

Re: Avoid PR72749 by not using unspecs

[Jakub Jelinek]

On Sun, Jan 15, 2017 at 12:37:19AM +1030, Alan Modra wrote:
> On Sat, Jan 14, 2017 at 02:46:11PM +0100, Jakub Jelinek wrote:
> > See Uros' comment about the INSN_CODE (insn) = insn_code_number;.
> 
> Later email retracted the one about a crash.
> 
> > Also, I'm worried about it for another reason, after the
> > if (!targetm.legitimate_combined_insn (insn))
> > call the PATTERN is reverted to something different, so keeping INSN_CODE
> > equal to insn_code_number (which we sometimes even change to -1)
> > looks wrong, if it is the right thing to do it for the
> > legitimate_combined_insn call, it should be reverted afterwards when the
> > PATTERN changes again.
> 
> It is reverted afterwards.
> 
>       PATTERN (insn) = old_pat;
>       REG_NOTES (insn) = old_notes;
>       INSN_CODE (insn) = old_icode;

Ah, you're right, I've missed that.  Also sorry for posting it in a wrong thread.

	Jakub

Re: PR79066, non-PIC code generated for powerpc glibc with -fpic

[Segher Boessenkool]

On Sun, Jan 15, 2017 at 12:08:39AM +1030, Alan Modra wrote:
> > Have you checked if/what this changes for some bigger code?
> 
> Well, the bootstrap was all langs (minus ada due to not having ada
> installed) and ppc32 multilibs were built.  Plus the testsuite run
> with RUNTESTFLAGS="--target_board=unix'{-m32,-m64}{-mlra,-mno-lra}'"
> 
> I would have bootstrapped -m32 except the machine I used lacked 32-bit
> gmp, mpfr, mpc and I was lazy.
> 
> > Okay for trunk if there is nothing unexpected.  Thanks!
> 
> I guess I should at least build glibc.

Yes exactly, something big that uses pic -- it is pretty obvious it won't
change anything for non-pic.

For gmp etc. you can use download_prequisites fwiw -- even lazier than
using the distro-provided binaries ;-)


Segher

Re: PR79066, non-PIC code generated for powerpc glibc with -fpic

[Alan Modra]

On Sat, Jan 14, 2017 at 09:19:52AM -0600, Segher Boessenkool wrote:
> On Sun, Jan 15, 2017 at 12:08:39AM +1030, Alan Modra wrote:
> > > Have you checked if/what this changes for some bigger code?
> > 
> > Well, the bootstrap was all langs (minus ada due to not having ada
> > installed) and ppc32 multilibs were built.  Plus the testsuite run
> > with RUNTESTFLAGS="--target_board=unix'{-m32,-m64}{-mlra,-mno-lra}'"
> > 
> > I would have bootstrapped -m32 except the machine I used lacked 32-bit
> > gmp, mpfr, mpc and I was lazy.
> > 
> > > Okay for trunk if there is nothing unexpected.  Thanks!
> > 
> > I guess I should at least build glibc.
> 
> Yes exactly, something big that uses pic -- it is pretty obvious it won't
> change anything for non-pic.

glibc built fine without elf_high/elf_low with no regressions in its
testsuite.  However, there is a problem in rs6000_emit_allocate_stack
-fstack-limit-symbol=SYMBOL code.  This now might ICE if someone tries
to use the option with -fpic/PIC.  I reckon the option combination to
be little used, so it shouldn't hurt to disable -fstack-limit-symbol
for PIC.  (We were generating non-PIC for the trap, so we probably
would have gotten a complaint about text relocs in shared libraries.)

This revised patch has been bootstrapped and regression tested as
before, and tested with glibc too.  OK?

	PR target/79066
	* config/rs6000/rs6000.md (elf_high, elf_low): Disable when pic.
	* config/rs6000/rs6000.c (rs6000_emit_allocate_stack): Don't allow
	symbolic stack limit when pic.
testsuite/
	* gcc.target/powerpc/pr79066.c: New.

diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index 11394b2..2dd6bbe 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -27668,7 +27668,8 @@ rs6000_emit_allocate_stack (HOST_WIDE_INT size, rtx copy_reg, int copy_off)
 	}
       else if (GET_CODE (stack_limit_rtx) == SYMBOL_REF
 	       && TARGET_32BIT
-	       && DEFAULT_ABI == ABI_V4)
+	       && DEFAULT_ABI == ABI_V4
+	       && !flag_pic)
 	{
 	  rtx toload = gen_rtx_CONST (VOIDmode,
 				      gen_rtx_PLUS (Pmode,
diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
index b333474..f00334a 100644
--- a/gcc/config/rs6000/rs6000.md
+++ b/gcc/config/rs6000/rs6000.md
@@ -10581,14 +10581,14 @@ (define_insn_and_split "*tocref<mode>"
 (define_insn "elf_high"
   [(set (match_operand:SI 0 "gpc_reg_operand" "=b*r")
 	(high:SI (match_operand 1 "" "")))]
-  "TARGET_ELF && ! TARGET_64BIT"
+  "TARGET_ELF && !TARGET_64BIT && !flag_pic"
   "lis %0,%1@ha")
 
 (define_insn "elf_low"
   [(set (match_operand:SI 0 "gpc_reg_operand" "=r")
 	(lo_sum:SI (match_operand:SI 1 "gpc_reg_operand" "b")
 		   (match_operand 2 "" "")))]
-   "TARGET_ELF && ! TARGET_64BIT"
+   "TARGET_ELF && !TARGET_64BIT && !flag_pic"
    "la %0,%2@l(%1)")
 \f
 ;; Call and call_value insns
diff --git a/gcc/testsuite/gcc.target/powerpc/pr79066.c b/gcc/testsuite/gcc.target/powerpc/pr79066.c
new file mode 100644
index 0000000..86b2014
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr79066.c
@@ -0,0 +1,14 @@
+/* { dg-do compile { target { fpic && ilp32 } } } */
+/* { dg-options "-O2 -fpic" } */
+/* { dg-final { scan-assembler-not "lis.*@ha" } } */
+
+union U { double x; int i[2]; };
+
+double
+foo (double x)
+{
+  union U v;
+  v.i[0] = 0x7ff00000;
+  v.i[1] = 0;
+  return x / v.x;
+}


-- 
Alan Modra
Australia Development Lab, IBM

Re: PR79066, non-PIC code generated for powerpc glibc with -fpic

[Segher Boessenkool]

On Mon, Jan 16, 2017 at 03:50:01PM +1030, Alan Modra wrote:
> > > > Okay for trunk if there is nothing unexpected.  Thanks!
> > > 
> > > I guess I should at least build glibc.
> > 
> > Yes exactly, something big that uses pic -- it is pretty obvious it won't
> > change anything for non-pic.
> 
> glibc built fine without elf_high/elf_low with no regressions in its
> testsuite.

I meant comparing the generated code, sorry.

> However, there is a problem in rs6000_emit_allocate_stack
> -fstack-limit-symbol=SYMBOL code.  This now might ICE if someone tries
> to use the option with -fpic/PIC.  I reckon the option combination to
> be little used, so it shouldn't hurt to disable -fstack-limit-symbol
> for PIC.  (We were generating non-PIC for the trap, so we probably
> would have gotten a complaint about text relocs in shared libraries.)
> 
> This revised patch has been bootstrapped and regression tested as
> before, and tested with glibc too.  OK?
> 
> 	PR target/79066
> 	* config/rs6000/rs6000.md (elf_high, elf_low): Disable when pic.
> 	* config/rs6000/rs6000.c (rs6000_emit_allocate_stack): Don't allow
> 	symbolic stack limit when pic.
> testsuite/
> 	* gcc.target/powerpc/pr79066.c: New.
> 
> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
> index 11394b2..2dd6bbe 100644
> --- a/gcc/config/rs6000/rs6000.c
> +++ b/gcc/config/rs6000/rs6000.c
> @@ -27668,7 +27668,8 @@ rs6000_emit_allocate_stack (HOST_WIDE_INT size, rtx copy_reg, int copy_off)
>  	}
>        else if (GET_CODE (stack_limit_rtx) == SYMBOL_REF
>  	       && TARGET_32BIT
> -	       && DEFAULT_ABI == ABI_V4)
> +	       && DEFAULT_ABI == ABI_V4
> +	       && !flag_pic)
>  	{
>  	  rtx toload = gen_rtx_CONST (VOIDmode,
>  				      gen_rtx_PLUS (Pmode,

Please at least make it sorry() for ABI_V4 && flag_pic.  Or what does it
result in with the patch as-is?

Okay with that change (if needed).  Thanks!


Segher

Re: PR79066, non-PIC code generated for powerpc glibc with -fpic

[Alan Modra]

On Mon, Jan 16, 2017 at 01:49:36PM -0600, Segher Boessenkool wrote:
> On Mon, Jan 16, 2017 at 03:50:01PM +1030, Alan Modra wrote:
> > > > > Okay for trunk if there is nothing unexpected.  Thanks!
> > > > 
> > > > I guess I should at least build glibc.
> > > 
> > > Yes exactly, something big that uses pic -- it is pretty obvious it won't
> > > change anything for non-pic.
> > 
> > glibc built fine without elf_high/elf_low with no regressions in its
> > testsuite.
> 
> I meant comparing the generated code, sorry.

libc.so had differences in debug sections due to libgcc source paths
being different, but other than that there were no differences.

> > However, there is a problem in rs6000_emit_allocate_stack
> > -fstack-limit-symbol=SYMBOL code.  This now might ICE if someone tries
> > to use the option with -fpic/PIC.  I reckon the option combination to
> > be little used, so it shouldn't hurt to disable -fstack-limit-symbol
> > for PIC.  (We were generating non-PIC for the trap, so we probably
> > would have gotten a complaint about text relocs in shared libraries.)
> > 
> > This revised patch has been bootstrapped and regression tested as
> > before, and tested with glibc too.  OK?
> > 
> > 	PR target/79066
> > 	* config/rs6000/rs6000.md (elf_high, elf_low): Disable when pic.
> > 	* config/rs6000/rs6000.c (rs6000_emit_allocate_stack): Don't allow
> > 	symbolic stack limit when pic.
> > testsuite/
> > 	* gcc.target/powerpc/pr79066.c: New.
> > 
> > diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
> > index 11394b2..2dd6bbe 100644
> > --- a/gcc/config/rs6000/rs6000.c
> > +++ b/gcc/config/rs6000/rs6000.c
> > @@ -27668,7 +27668,8 @@ rs6000_emit_allocate_stack (HOST_WIDE_INT size, rtx copy_reg, int copy_off)
> >  	}
> >        else if (GET_CODE (stack_limit_rtx) == SYMBOL_REF
> >  	       && TARGET_32BIT
> > -	       && DEFAULT_ABI == ABI_V4)
> > +	       && DEFAULT_ABI == ABI_V4
> > +	       && !flag_pic)
> >  	{
> >  	  rtx toload = gen_rtx_CONST (VOIDmode,
> >  				      gen_rtx_PLUS (Pmode,
> 
> Please at least make it sorry() for ABI_V4 && flag_pic.  Or what does it
> result in with the patch as-is?

I think we're OK as is.  A warning is emitted "stack limit expression
is not supported".

-- 
Alan Modra
Australia Development Lab, IBM

Re: Avoid PR72749 by not using unspecs

[Alan Modra]

On Sat, Jan 14, 2017 at 03:51:27AM -0600, Segher Boessenkool wrote:
> It also prevents combine from combining any instruction into an existing
> doloop insn (resulting in a doloop insn again).  This isn't too serious,
> almost always this is just a register copy that will be optimised away
> some other way.

Right.  I forgot to mention, I looked at all of gcc/*.o before/after
the patch and saw no occurrences of any missed combines.  Just the
expected changes in rs6000.o, various insn-*.o from twiddling
rs6000.md, and one constant change in read-rtl-function.o.

-- 
Alan Modra
Australia Development Lab, IBM

Re: Avoid PR72749 by not using unspecs

[Uros Bizjak]

On Sat, Jan 14, 2017 at 11:19 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Fri, Jan 13, 2017 at 4:58 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
>> On Fri, Jan 13, 2017 at 12:50 PM, Alan Modra <amodra@gmail.com> wrote:
>>> Rather than using unspecs in doloop insns to stop combine creating
>>> these insns, use legitimate_combined_insn.
>>>
>>> I'm not sure why the original patch implementing
>>> legitimate_combined_insn did not store the result of recog to the insn
>>> but it seems good to me, and would allow the recog call in
>>> ix86_legitimate_combined_insn to be omitted.  (I tested that too, not
>>> shown here.)
>>
>> IIRC, I copied operand scanning loop from recog.c (around line 2580)
>> and the function was later enhanced with preferred alternatives
>> handling. The function worked well, and not being an expert in this
>> area, I didn't try to "optimize" the code that worked...
>>
>> So, there is no particular reason for the current implementation.
>
> FYI, the following patch immediately crashed build:

Bah, it would help if I actually saved source with proposed combine.c change.

Testing the patch...

Uros.

Re: Avoid PR72749 by not using unspecs

[Uros Bizjak]

On Fri, Jan 13, 2017 at 4:58 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Fri, Jan 13, 2017 at 12:50 PM, Alan Modra <amodra@gmail.com> wrote:
>> Rather than using unspecs in doloop insns to stop combine creating
>> these insns, use legitimate_combined_insn.
>>
>> I'm not sure why the original patch implementing
>> legitimate_combined_insn did not store the result of recog to the insn
>> but it seems good to me, and would allow the recog call in
>> ix86_legitimate_combined_insn to be omitted.  (I tested that too, not
>> shown here.)
>
> IIRC, I copied operand scanning loop from recog.c (around line 2580)
> and the function was later enhanced with preferred alternatives
> handling. The function worked well, and not being an expert in this
> area, I didn't try to "optimize" the code that worked...
>
> So, there is no particular reason for the current implementation.

FYI, the following patch immediately crashed build:

--cut here--
Index: i386.c
===================================================================
--- i386.c      (revision 244463)
+++ i386.c      (working copy)
@@ -8130,7 +8130,7 @@ ix86_legitimate_combined_insn (rtx_insn *insn)
      generating insn patterns with invalid hard register operands.
      These invalid insns can eventually confuse reload to error out
      with a spill failure.  See also PRs 46829 and 46843.  */
-  if ((INSN_CODE (insn) = recog (PATTERN (insn), insn, 0)) >= 0)
+  if (INSN_CODE (insn) >= 0)
     {
       int i;

--cut here--

/home/uros/gcc-svn/trunk/libgcc/libgcc2.c:557:1: internal compiler
error: Segmentation fault
 }
 ^
0xe82bcc crash_signal
        /home/uros/gcc-svn/trunk/gcc/toplev.c:333
0x1621444 insn_extract(rtx_insn*)
        /ssd/uros/gcc-build/gcc/insn-extract.c:7453
0xd9e345 extract_insn(rtx_insn*)
        /home/uros/gcc-svn/trunk/gcc/recog.c:2317
0x126e662 ix86_legitimate_combined_insn
        /home/uros/gcc-svn/trunk/gcc/config/i386/i386.c:8137
0x1814568 recog_for_combine_1
        /home/uros/gcc-svn/trunk/gcc/combine.c:11206
0x1814d73 recog_for_combine
        /home/uros/gcc-svn/trunk/gcc/combine.c:11363
0x17ffb69 try_combine
        /home/uros/gcc-svn/trunk/gcc/combine.c:3824
0x17f8d7e combine_instructions
        /home/uros/gcc-svn/trunk/gcc/combine.c:1372
0x181cb67 rest_of_handle_combine
        /home/uros/gcc-svn/trunk/gcc/combine.c:14607
0x181cc28 execute
        /home/uros/gcc-svn/trunk/gcc/combine.c:14652

It looks that recog_for_combine doesn't do everything recog does.

Uros.

Re: Avoid PR72749 by not using unspecs

[Segher Boessenkool]

On Fri, Jan 13, 2017 at 10:20:58PM +1030, Alan Modra wrote:
> Rather than using unspecs in doloop insns to stop combine creating
> these insns, use legitimate_combined_insn.
> 
> I'm not sure why the original patch implementing
> legitimate_combined_insn did not store the result of recog to the insn
> but it seems good to me, and would allow the recog call in
> ix86_legitimate_combined_insn to be omitted.  (I tested that too, not
> shown here.)

It is always restored after the hook call, so this change is fine.

> +      /* Reject creating doloop insns.  Combine should not be allowed
> +	 to create these for a number of reasons:

It also prevents combine from combining any instruction into an existing
doloop insn (resulting in a doloop insn again).  This isn't too serious,
almost always this is just a register copy that will be optimised away
some other way.  The unspec hack has more significant problems.

> +	 3) Faced with not being able to allocate ctr for ctrsi/crtdi
> +	 insns, the register allocator sometimes uses floating point
> +	 or vector registers for the pseudo.  Since ctrsi/ctrdi is a
> +	 jump insn and output reloads are not implemented for jumps,
> +	 the ctrsi/ctrdi splitters need to handle all possible cases.
> +	 That's a pain, and it gets to be seriously difficult when a
> +	 splitter that runs after reload needs memory to transfer from
> +	 a gpr to fpr.  See PR70098 and PR71763 which are not fixed
> +	 for the difficult case.  It's better to not create problems
> +	 in the first place.  */

Yeah :-)  Note that the problems still can happen, just much less frequently.

> @@ -12751,9 +12749,8 @@ (define_expand "ctr<mode>"
>  ;; JUMP_INSNs.
>  ;; For the length attribute to be calculated correctly, the
>  ;; label MUST be operand 0.
> -;; The UNSPEC is present to prevent combine creating this pattern.

Maybe add a note here that rs6000_legitimate_combined_insn will refuse
to combine to this?

Okay for trunk.  Thanks!


Segher

Re: Avoid PR72749 by not using unspecs

[Jeff Law]

On 01/13/2017 04:50 AM, Alan Modra wrote:
> Rather than using unspecs in doloop insns to stop combine creating
> these insns, use legitimate_combined_insn.
>
> I'm not sure why the original patch implementing
> legitimate_combined_insn did not store the result of recog to the insn
> but it seems good to me, and would allow the recog call in
> ix86_legitimate_combined_insn to be omitted.  (I tested that too, not
> shown here.)
>
> Bootstrapped and regression tested powerpc64le-linux, powerpc64-linux,
> x86_64-linux.  OK for mainline?
>
> 	PR target/72749
> 	* combine.c (recog_for_combine_1): Set INSN_CODE before calling
> 	target legitimate_combined_insn.
> 	* config/rs6000/rs6000.c (TARGET_LEGITIMATE_COMBINED_INSN): Define.
> 	(rs6000_legitimate_combined_insn): New function.
> 	* config/rs6000/rs6000.md (UNSPEC_DOLOOP): Delete, and remove
> 	all uses.
> 	(ctr<mode>_internal3): Rename from *ctr<mode>_internal5.
> 	(ctr<mode>_internal4): Rename from *ctr<mode>_internal6.
> 	(ctr<mode>_internal1, ctr<mode>_internal2): Remove '*' from name.
OK on the combine.c changes.  ppc maintainers own the rest.

jeff

Re: Avoid PR72749 by not using unspecs

[Uros Bizjak]

On Fri, Jan 13, 2017 at 12:50 PM, Alan Modra <amodra@gmail.com> wrote:
> Rather than using unspecs in doloop insns to stop combine creating
> these insns, use legitimate_combined_insn.
>
> I'm not sure why the original patch implementing
> legitimate_combined_insn did not store the result of recog to the insn
> but it seems good to me, and would allow the recog call in
> ix86_legitimate_combined_insn to be omitted.  (I tested that too, not
> shown here.)

IIRC, I copied operand scanning loop from recog.c (around line 2580)
and the function was later enhanced with preferred alternatives
handling. The function worked well, and not being an expert in this
area, I didn't try to "optimize" the code that worked...

So, there is no particular reason for the current implementation.

Uros.

Avoid PR72749 by not using unspecs

[Alan Modra]

Rather than using unspecs in doloop insns to stop combine creating
these insns, use legitimate_combined_insn.

I'm not sure why the original patch implementing
legitimate_combined_insn did not store the result of recog to the insn
but it seems good to me, and would allow the recog call in
ix86_legitimate_combined_insn to be omitted.  (I tested that too, not
shown here.)

Bootstrapped and regression tested powerpc64le-linux, powerpc64-linux,
x86_64-linux.  OK for mainline?

	PR target/72749
	* combine.c (recog_for_combine_1): Set INSN_CODE before calling
	target legitimate_combined_insn.
	* config/rs6000/rs6000.c (TARGET_LEGITIMATE_COMBINED_INSN): Define.
	(rs6000_legitimate_combined_insn): New function.
	* config/rs6000/rs6000.md (UNSPEC_DOLOOP): Delete, and remove
	all uses.
	(ctr<mode>_internal3): Rename from *ctr<mode>_internal5.
	(ctr<mode>_internal4): Rename from *ctr<mode>_internal6.
	(ctr<mode>_internal1, ctr<mode>_internal2): Remove '*' from name.

diff --git a/gcc/combine.c b/gcc/combine.c
index 3043f2a..73a74ac 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -11199,6 +11199,7 @@ recog_for_combine_1 (rtx *pnewpat, rtx_insn *insn, rtx *pnotes)
       old_icode = INSN_CODE (insn);
       PATTERN (insn) = pat;
       REG_NOTES (insn) = notes;
+      INSN_CODE (insn) = insn_code_number;
 
       /* Allow targets to reject combined insn.  */
       if (!targetm.legitimate_combined_insn (insn))
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index 02b521c..76ba81b 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -1558,6 +1558,9 @@ static const struct attribute_spec rs6000_attribute_table[] =
 #undef TARGET_CONST_NOT_OK_FOR_DEBUG_P
 #define TARGET_CONST_NOT_OK_FOR_DEBUG_P rs6000_const_not_ok_for_debug_p
 
+#undef TARGET_LEGITIMATE_COMBINED_INSN
+#define TARGET_LEGITIMATE_COMBINED_INSN rs6000_legitimate_combined_insn
+
 #undef TARGET_ASM_FUNCTION_PROLOGUE
 #define TARGET_ASM_FUNCTION_PROLOGUE rs6000_output_function_prologue
 #undef TARGET_ASM_FUNCTION_EPILOGUE
@@ -9076,6 +9079,49 @@ rs6000_const_not_ok_for_debug_p (rtx x)
   return false;
 }
 
+
+/* Implement the TARGET_LEGITIMATE_COMBINED_INSN hook.  */
+
+static bool
+rs6000_legitimate_combined_insn (rtx_insn *insn)
+{
+  switch (INSN_CODE (insn))
+    {
+      /* Reject creating doloop insns.  Combine should not be allowed
+	 to create these for a number of reasons:
+	 1) In a nested loop, if combine creates one of these in an
+	 outer loop and the register allocator happens to allocate ctr
+	 to the outer loop insn, then the inner loop can't use ctr.
+	 Inner loops ought to be more highly optimized.
+	 2) Combine often wants to create one of these from what was
+	 originally a three insn sequence, first combining the three
+	 insns to two, then to ctrsi/ctrdi.  When ctrsi/ctrdi is not
+	 allocated ctr, the splitter takes use back to the three insn
+	 sequence.  It's better to stop combine at the two insn
+	 sequence.
+	 3) Faced with not being able to allocate ctr for ctrsi/crtdi
+	 insns, the register allocator sometimes uses floating point
+	 or vector registers for the pseudo.  Since ctrsi/ctrdi is a
+	 jump insn and output reloads are not implemented for jumps,
+	 the ctrsi/ctrdi splitters need to handle all possible cases.
+	 That's a pain, and it gets to be seriously difficult when a
+	 splitter that runs after reload needs memory to transfer from
+	 a gpr to fpr.  See PR70098 and PR71763 which are not fixed
+	 for the difficult case.  It's better to not create problems
+	 in the first place.  */
+    case CODE_FOR_ctrsi_internal1:
+    case CODE_FOR_ctrdi_internal1:
+    case CODE_FOR_ctrsi_internal2:
+    case CODE_FOR_ctrdi_internal2:
+    case CODE_FOR_ctrsi_internal3:
+    case CODE_FOR_ctrdi_internal3:
+    case CODE_FOR_ctrsi_internal4:
+    case CODE_FOR_ctrdi_internal4:
+      return false;
+    }
+  return true;
+}
+
 /* Construct the SYMBOL_REF for the tls_get_addr function.  */
 
 static GTY(()) rtx rs6000_tls_symbol;
diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
index f7c1ab2..9442b07 100644
--- a/gcc/config/rs6000/rs6000.md
+++ b/gcc/config/rs6000/rs6000.md
@@ -149,7 +149,6 @@ (define_c_enum "unspec"
    UNSPEC_IEEE128_MOVE
    UNSPEC_IEEE128_CONVERT
    UNSPEC_SIGNBIT
-   UNSPEC_DOLOOP
    UNSPEC_SF_FROM_SI
    UNSPEC_SI_FROM_SF
   ])
@@ -12740,7 +12739,6 @@ (define_expand "ctr<mode>"
 	      (set (match_dup 0)
 		   (plus:P (match_dup 0)
 			    (const_int -1)))
-	      (unspec [(const_int 0)] UNSPEC_DOLOOP)
 	      (clobber (match_scratch:CC 2 ""))
 	      (clobber (match_scratch:P 3 ""))])]
   ""
@@ -12751,9 +12749,8 @@ (define_expand "ctr<mode>"
 ;; JUMP_INSNs.
 ;; For the length attribute to be calculated correctly, the
 ;; label MUST be operand 0.
-;; The UNSPEC is present to prevent combine creating this pattern.
 
-(define_insn "*ctr<mode>_internal1"
+(define_insn "ctr<mode>_internal1"
   [(set (pc)
 	(if_then_else (ne (match_operand:P 1 "register_operand" "c,*b,*b,*b")
 			  (const_int 1))
@@ -12762,7 +12759,6 @@ (define_insn "*ctr<mode>_internal1"
    (set (match_operand:P 2 "nonimmediate_operand" "=1,*r,m,*d*wi*c*l")
 	(plus:P (match_dup 1)
 		 (const_int -1)))
-   (unspec [(const_int 0)] UNSPEC_DOLOOP)
    (clobber (match_scratch:CC 3 "=X,&x,&x,&x"))
    (clobber (match_scratch:P 4 "=X,X,&r,r"))]
   ""
@@ -12778,7 +12774,7 @@ (define_insn "*ctr<mode>_internal1"
   [(set_attr "type" "branch")
    (set_attr "length" "*,16,20,20")])
 
-(define_insn "*ctr<mode>_internal2"
+(define_insn "ctr<mode>_internal2"
   [(set (pc)
 	(if_then_else (ne (match_operand:P 1 "register_operand" "c,*b,*b,*b")
 			  (const_int 1))
@@ -12787,7 +12783,6 @@ (define_insn "*ctr<mode>_internal2"
    (set (match_operand:P 2 "nonimmediate_operand" "=1,*r,m,*d*wi*c*l")
 	(plus:P (match_dup 1)
 		 (const_int -1)))
-   (unspec [(const_int 0)] UNSPEC_DOLOOP)
    (clobber (match_scratch:CC 3 "=X,&x,&x,&x"))
    (clobber (match_scratch:P 4 "=X,X,&r,r"))]
   ""
@@ -12805,7 +12800,7 @@ (define_insn "*ctr<mode>_internal2"
 
 ;; Similar but use EQ
 
-(define_insn "*ctr<mode>_internal5"
+(define_insn "ctr<mode>_internal3"
   [(set (pc)
 	(if_then_else (eq (match_operand:P 1 "register_operand" "c,*b,*b,*b")
 			  (const_int 1))
@@ -12814,7 +12809,6 @@ (define_insn "*ctr<mode>_internal5"
    (set (match_operand:P 2 "nonimmediate_operand" "=1,*r,m,*d*wi*c*l")
 	(plus:P (match_dup 1)
 		 (const_int -1)))
-   (unspec [(const_int 0)] UNSPEC_DOLOOP)
    (clobber (match_scratch:CC 3 "=X,&x,&x,&x"))
    (clobber (match_scratch:P 4 "=X,X,&r,r"))]
   ""
@@ -12830,7 +12824,7 @@ (define_insn "*ctr<mode>_internal5"
   [(set_attr "type" "branch")
    (set_attr "length" "*,16,20,20")])
 
-(define_insn "*ctr<mode>_internal6"
+(define_insn "ctr<mode>_internal4"
   [(set (pc)
 	(if_then_else (eq (match_operand:P 1 "register_operand" "c,*b,*b,*b")
 			  (const_int 1))
@@ -12839,7 +12833,6 @@ (define_insn "*ctr<mode>_internal6"
    (set (match_operand:P 2 "nonimmediate_operand" "=1,*r,m,*d*wi*c*l")
 	(plus:P (match_dup 1)
 		 (const_int -1)))
-   (unspec [(const_int 0)] UNSPEC_DOLOOP)
    (clobber (match_scratch:CC 3 "=X,&x,&x,&x"))
    (clobber (match_scratch:P 4 "=X,X,&r,r"))]
   ""
@@ -12866,7 +12859,6 @@ (define_split
 		      (match_operand 6 "" "")))
    (set (match_operand:P 0 "int_reg_operand" "")
 	(plus:P (match_dup 1) (const_int -1)))
-   (unspec [(const_int 0)] UNSPEC_DOLOOP)
    (clobber (match_scratch:CC 3 ""))
    (clobber (match_scratch:P 4 ""))]
   "reload_completed"
@@ -12892,7 +12884,6 @@ (define_split
 		      (match_operand 6 "" "")))
    (set (match_operand:P 0 "nonimmediate_operand" "")
 	(plus:P (match_dup 1) (const_int -1)))
-   (unspec [(const_int 0)] UNSPEC_DOLOOP)
    (clobber (match_scratch:CC 3 ""))
    (clobber (match_scratch:P 4 ""))]
   "reload_completed && ! gpc_reg_operand (operands[0], SImode)"
diff --git a/gcc/testsuite/gcc.c-torture/compile/pr72749.c b/gcc/testsuite/gcc.c-torture/compile/pr72749.c
new file mode 100644
index 0000000..2ef4d9a
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/compile/pr72749.c
@@ -0,0 +1,21 @@
+/* { dg-options "-O2 -fsched2-use-superblocks" } */
+
+int as;
+
+void
+ji (int *x4)
+{
+  if (0)
+    {
+      unsigned int pv;
+
+      while (as < 0)
+        {
+          for (*x4 = 0; *x4 < 1; ++(*x4))
+yj:
+            x4 = (int *)&pv;
+          ++as;
+        }
+    }
+  goto yj;
+}

-- 
Alan Modra
Australia Development Lab, IBM