Patchwork [rtl-optimization] : Fix PR 51821, 64bit > 32bit conversion produces incorrect results with optimizations

login
register
mail settings
Submitter Uros Bizjak
Date Jan. 12, 2012, 7:14 p.m.
Message ID <CAFULd4ZEJa7XFOxOa86+rN202QqyBa6D6k-vwPBxaf24Ek1XAw@mail.gmail.com>
Download mbox | patch
Permalink /patch/135682/
State New
Headers show

Comments

Uros Bizjak - Jan. 12, 2012, 7:14 p.m.
Hello!

As discussed in the PR, attached testcase uncovers problem with the
way peephole2 pas determines free registers to allocate temporary
register. The problem was with the pattern such as:

(insn 7 19 16 2 (parallel [
            (set (reg:DI 0 ax [65])
                (ashift:DI (const_int -1 [0xffffffffffffffff])
                    (reg:QI 2 cx [orig:63 shift_size ] [63])))
            (clobber (reg:CC 17 flags))
        ]) pr51821.c:8 489 {*ashldi3_doubleword}
     (expr_list:REG_DEAD (reg:QI 2 cx [orig:63 shift_size ] [63])
        (expr_list:REG_UNUSED (reg:CC 17 flags)
            (expr_list:REG_UNUSED (reg:SI 1 dx)
                (expr_list:REG_EQUAL (ashift:DI (const_int -1
[0xffffffffffffffff])
                        (subreg:QI (reg/v:SI 2 cx [orig:63 shift_size
] [63]) 0))
                    (nil))))))

where DImode ax/dx register pair gets defined, while at the same time,
one of the registers from the register pair gets marked as unused.
Currently, peephole2 pass determines clobbered registers from DF life
analysis, as the difference of registers, live before the insn
sequence and after insn sequence. Unfortunately, as shown by attached
testcase in the PR, this approach is not correct. In the above
pattern, dx is considered "dead" due to (REG_UNUSED dx) note.

The solution is to fix the scanning loop to look into the insn pattern
itself for all set and clobbered hard registers. This way, all
registers, clobbered by the pattern, will be correctly marked in the
"live" bitmap, including FLAGS register that is ignored by current
approach.

2012-01-12  Uros Bizjak  <ubizjak@gmail.com>

	* recog.c (peep2_find_free_register): Determine clobbered registers
	from insn pattern.

testsuite/ChangeLog:

2012-01-12  Uros Bizjak  <ubizjak@gmail.com>

	* gcc.dg/pr51821.c: New test.

Patch was bootstrapped and regression tested on x86_64-pc-linux-gnu.

OK for mainline and release branches?

Uros.
Eric Botcazou - Jan. 12, 2012, 11:01 p.m.
> The solution is to fix the scanning loop to look into the insn pattern
> itself for all set and clobbered hard registers. This way, all
> registers, clobbered by the pattern, will be correctly marked in the
> "live" bitmap, including FLAGS register that is ignored by current
> approach.
>
> 2012-01-12  Uros Bizjak  <ubizjak@gmail.com>
>
> 	* recog.c (peep2_find_free_register): Determine clobbered registers
> 	from insn pattern.

Is that a complete solution though?  Don't we need to do more, for example 
because of peep2_reg_dead_p and peep2_update_life?  These are not rhetorical 
questions, but genuine ones; it's a little disturbing to discover such a flaw 
in this kind of code after all these years (I can reproduce the problem with 
all the compilers of the 4.x series, so this didn't work with flow.c either).

Thanks for debugging this in any case.
Uros Bizjak - Jan. 13, 2012, 8:11 a.m.
On Fri, Jan 13, 2012 at 12:01 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> The solution is to fix the scanning loop to look into the insn pattern
>> itself for all set and clobbered hard registers. This way, all
>> registers, clobbered by the pattern, will be correctly marked in the
>> "live" bitmap, including FLAGS register that is ignored by current
>> approach.
>>
>> 2012-01-12  Uros Bizjak  <ubizjak@gmail.com>
>>
>>       * recog.c (peep2_find_free_register): Determine clobbered registers
>>       from insn pattern.
>
> Is that a complete solution though?  Don't we need to do more, for example
> because of peep2_reg_dead_p and peep2_update_life?  These are not rhetorical
> questions, but genuine ones; it's a little disturbing to discover such a flaw
> in this kind of code after all these years (I can reproduce the problem with
> all the compilers of the 4.x series, so this didn't work with flow.c either).

Yes, it is a complete solution. Tracking register liveness is
different issue, and a register is indeed dead after instruction, if
it has been clobbered by insn, or when marked unused. This works OK,
and there are many examples of peep2_{reg|regno}_dead_p usag in
i386.md.

The problem my patch solves is the answer to the question "Is the
choosen non-live temporary register untouched over the insn
sequence?". The answer: "Yes, if it was not set or clobbered by any
insn in the sequence".

Uros.
Eric Botcazou - Jan. 13, 2012, 9 a.m.
> Yes, it is a complete solution. Tracking register liveness is
> different issue, and a register is indeed dead after instruction, if
> it has been clobbered by insn, or when marked unused.

My concern was liveness within the new sequence of instructions: suppose you 
have

  (set (reg:M x) (...))  REG_UNUSED (reg:M x)

and we "peephole" the instruction.  Is the live range of (reg:M x) properly 
extended within the new sequence of instructions?

> The problem my patch solves is the answer to the question "Is the
> choosen non-live temporary register untouched over the insn
> sequence?". The answer: "Yes, if it was not set or clobbered by any
> insn in the sequence".

Are you sure that you don't need to do this in addition to the existing test, 
instead of in lieu of the existing test?
Uros Bizjak - Jan. 13, 2012, 9:23 a.m.
On Fri, Jan 13, 2012 at 10:00 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Yes, it is a complete solution. Tracking register liveness is
>> different issue, and a register is indeed dead after instruction, if
>> it has been clobbered by insn, or when marked unused.
>
> My concern was liveness within the new sequence of instructions: suppose you
> have
>
>  (set (reg:M x) (...))  REG_UNUSED (reg:M x)
>
> and we "peephole" the instruction.  Is the live range of (reg:M x) properly
> extended within the new sequence of instructions?

The patch doesn't change this functionality, but:

(match_scratch:M 1 "r")
 (set (reg:M x) (...))  REG_UNUSED (reg:M x)
(match_dup 1)

Previously, register x would be allocated as a scracth register, but
now it isn't. Regarding the liveness analysis of a new sequence - this
functionality is not changed at all. As said, the new code only
prevents x to be allocated as a scratch that must live up to the
(match_dup 1).

>> The problem my patch solves is the answer to the question "Is the
>> choosen non-live temporary register untouched over the insn
>> sequence?". The answer: "Yes, if it was not set or clobbered by any
>> insn in the sequence".
>
> Are you sure that you don't need to do this in addition to the existing test,
> instead of in lieu of the existing test?

Yes, because new test ALWAYS includes the registers that were wrongly
marked as dead by previous test due to REG_UNUSED and noclobber
processing.

Uros.
Richard Sandiford - Jan. 13, 2012, 9:58 a.m.
Uros Bizjak <ubizjak@gmail.com> writes:
> On Fri, Jan 13, 2012 at 10:00 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>>> The problem my patch solves is the answer to the question "Is the
>>> choosen non-live temporary register untouched over the insn
>>> sequence?". The answer: "Yes, if it was not set or clobbered by any
>>> insn in the sequence".
>>
>> Are you sure that you don't need to do this in addition to the existing test,
>> instead of in lieu of the existing test?
>
> Yes, because new test ALWAYS includes the registers that were wrongly
> marked as dead by previous test due to REG_UNUSED and noclobber
> processing.

Yeah, the patch looks like the right fix to me FWIW.  Being able to use
DF_INSN_DEFS in this way is part of the point of having df: when going
from one insn to the next within the same bb, there are no "hidden"
influences that would cause a register to become live outside of
DF_INSN_DEFS.  The only requirement is that we call df_insn_rescan
on new instructions, and we do (in peep2_update_life, where we need
it for df_simulate_one_insn_backwards).

Or to put it another way: the insn-to-insn changes in the current liveness
sets are all produced by df_simulate_one_insn_{backwards,forwards}, which
uses the same information.  So I don't think there's any reason why we
need to keep the current code as well.

Richard
Paolo Bonzini - Jan. 13, 2012, 10:51 a.m.
On 01/13/2012 10:58 AM, Richard Sandiford wrote:
> >  Yes, because new test ALWAYS includes the registers that were wrongly
> >  marked as dead by previous test due to REG_UNUSED and noclobber
> >  processing.
>
> Or to put it another way: the insn-to-insn changes in the current liveness
> sets are all produced by df_simulate_one_insn_{backwards,forwards}, which
> uses the same information.  So I don't think there's any reason why we
> need to keep the current code as well.

Yes, both explanations are sound.  I think the patch is good, too.

Paolo
Uros Bizjak - Jan. 15, 2012, 6:14 p.m.
On Fri, Jan 13, 2012 at 10:58 AM, Richard Sandiford
<richard.sandiford@linaro.org> wrote:

>>>> The problem my patch solves is the answer to the question "Is the
>>>> choosen non-live temporary register untouched over the insn
>>>> sequence?". The answer: "Yes, if it was not set or clobbered by any
>>>> insn in the sequence".
>>>
>>> Are you sure that you don't need to do this in addition to the existing test,
>>> instead of in lieu of the existing test?
>>
>> Yes, because new test ALWAYS includes the registers that were wrongly
>> marked as dead by previous test due to REG_UNUSED and noclobber
>> processing.
>
> Yeah, the patch looks like the right fix to me FWIW.  Being able to use
> DF_INSN_DEFS in this way is part of the point of having df: when going
> from one insn to the next within the same bb, there are no "hidden"
> influences that would cause a register to become live outside of
> DF_INSN_DEFS.  The only requirement is that we call df_insn_rescan
> on new instructions, and we do (in peep2_update_life, where we need
> it for df_simulate_one_insn_backwards).
>
> Or to put it another way: the insn-to-insn changes in the current liveness
> sets are all produced by df_simulate_one_insn_{backwards,forwards}, which
> uses the same information.  So I don't think there's any reason why we
> need to keep the current code as well.

Patch was approved offline by Richard Sandiford, so committed to
mainline SVN, will be committed to release branches after bootstrap &
regression tests.

Thanks,
Uros.

Patch

Index: recog.c
===================================================================
--- recog.c	(revision 183130)
+++ recog.c	(working copy)
@@ -3038,6 +3038,7 @@  peep2_find_free_register (int from, int to, const
   static int search_ofs;
   enum reg_class cl;
   HARD_REG_SET live;
+  df_ref *def_rec;
   int i;
 
   gcc_assert (from < MAX_INSNS_PER_PEEP2 + 1);
@@ -3051,12 +3052,14 @@  peep2_find_free_register (int from, int to, const
 
   while (from != to)
     {
-      HARD_REG_SET this_live;
+      gcc_assert (peep2_insn_data[from].insn != NULL_RTX);
 
+      /* Don't use registers set or clobbered by the insn.  */
+      for (def_rec = DF_INSN_DEFS (peep2_insn_data[from].insn);
+	   *def_rec; def_rec++)
+	SET_HARD_REG_BIT (live, DF_REF_REGNO (*def_rec));
+
       from = peep2_buf_position (from + 1);
-      gcc_assert (peep2_insn_data[from].insn != NULL_RTX);
-      REG_SET_TO_HARD_REG_SET (this_live, peep2_insn_data[from].live_before);
-      IOR_HARD_REG_SET (live, this_live);
     }
 
   cl = (class_str[0] == 'r' ? GENERAL_REGS
Index: testsuite/gcc.dg/pr51821.c
===================================================================
--- testsuite/gcc.dg/pr51821.c	(revision 0)
+++ testsuite/gcc.dg/pr51821.c	(revision 0)
@@ -0,0 +1,24 @@ 
+/* { dg-do run } */
+/* { dg-options "-O2 -msse" { target { i?86-*-* x86_64-*-* } } } */
+/* { dg-require-effective-target sse_runtime { target { i?86-*-* x86_64-*-* } } } */
+
+extern void abort (void);
+
+unsigned int  __attribute__((noinline))
+test (int shift_size)
+{
+  unsigned long long res = ~0;
+
+  return res << shift_size;
+}
+
+int
+main ()
+{
+  int dst = 32;
+
+  if (test (dst) != 0)
+    abort ();
+
+  return 0;
+}