Re: Dataflow miscompiles soft-fp/floatuntitf.c on x86_64

Re: Dataflow miscompiles soft-fp/floatuntitf.c on x86_64

[Ian Lance Taylor]

Uros Bizjak <ubizjak@gmail.com> writes:

> Latest dataflow branch segfaults for:
> 
> FAIL: gcc.dg/torture/fp-int-convert-float128-timode.c  -O0  execution test
> FAIL: gcc.dg/torture/fp-int-convert-float128-timode.c  -O1  execution test
> FAIL: gcc.dg/torture/fp-int-convert-float128-timode.c  -O2  execution test
> FAIL: gcc.dg/torture/fp-int-convert-float128-timode.c  -O3
> -fomit-frame-pointer  execution test
> FAIL: gcc.dg/torture/fp-int-convert-float128-timode.c  -O3 -g
> execution test
> FAIL: gcc.dg/torture/fp-int-convert-float128-timode.c  -Os  execution test
> 
> (The tests were run on FC6, x86_64-pc-linux-gnu).
> 
> This is due to segfault in soft-fp/floatuntitf.c at __floatuntitf+202,
> where %rsp is not aligned to 16 bytes for movdqa:
> 
> 0x0000000000402c10 <__floatuntitf+192>: mov    %ax,0xfffffffffffffffe(%rsp)
> 0x0000000000402c15 <__floatuntitf+197>: andb
> $0x7f,0xffffffffffffffff(%rsp)
> 0x0000000000402c1a <__floatuntitf+202>: movdqa
> 0xfffffffffffffff0(%rsp),%xmm0   <<<
> 0x0000000000402c20 <__floatuntitf+208>: pop    %rbx
> 0x0000000000402c21 <__floatuntitf+209>: pop    %rbp
> 0x0000000000402c22 <__floatuntitf+210>: retq  Uros.


Seongbae tracked this down.  It's an interesting problem.

In the .lreg dump we see this:

;; Register 79 in 5.

...

(insn:HI 4 3 5 2 ../../../gccDBaseline/libgcc/../gcc/config/soft-fp/floatuntitf.c:35 (set (reg:TI 81 [ i ])
        (subreg:TI (reg:DI 79 [ i ]) 0)) 87 {*movti_rex64} (expr_list:REG_DEAD (reg:DI 79 [ i ])
        (nil)))

Note that paradoxical subeg applied to reg 79.  Note that pseudo reg
79 is put in hard reg 5 (%rdi).

When reload starts, it calls mark_home_live to call
df_set_regs_ever_live for each hard register being used.  Since reg 79
is DImode, and has been put in hard reg 5 this marks hard reg 5 as
live (this is x86_64 so hard reg 5 is DImode).

reload proceeds as usual.  At the end, reload calls
cleanup_subreg_operands.  That function fixes up any subregs of hard
registers to refer directly to the register in question.  In this
case, though, it turns the paradoxical subreg into a TImode reference
to hard reg 5, giving us this in the .greg dump:

(insn:HI 4 7 5 2 ../../../gccDBaseline/libgcc/../gcc/config/soft-fp/floatuntitf.c:35 (set (reg:TI 37 r8 [orig:81 i ] [81])
        (reg:TI 5 di [orig:79 i ] [79])) 87 {*movti_rex64} (nil))

So now we have a TImode reference to hard reg 5, which means hard regs
5 and 6.  But when we set regs_ever_live, we only marked 5.  As it
happens, reg 6 was not otherwise referenced in the function.  And, as
it happens, reg 6 is %rbp, which is callee saved.

All of the above happens on both mainline and dataflow branch.

On mainline we simply carry on, with a reference to hard reg 6 but
with that bit clear in regs_ever_live.  Since this is paradoxical
subreg, hard reg 6 is never actually changed, and indeed all the
references to it drop out as well.  So mainline is fine.

Dataflow branch, however, recomputes regs_ever_live after reload is
finished.  When it does this, it sees the TImode reference to reg 5,
and therefore marks both reg 5 and 6 as live.

But it only does this after reload is finished.  The effect is that
the value of regs_ever_live is not the same during and after reload.
reg 6 is clear in regs_ever_live during reload, and set after reload.

This is bad.  It's bad because the x86_64 backend uses regs_ever_live
when determining the frame layout, which is done when computing
register elimination during reload.  And the x86_64 backend uses
regs_ever_live again when deciding which registers to save and restore
in the prologue and epilogue, after reload.  When the frame layout
computation changes, it means that the stack usage changes, and the
register elimination computations are incorrect.

Fortunately the fix turns out to be relatively simple.  I have
appended the untested patch.

Note that this means that dataflow branch will generate worse code for
this test case: it will save %rbp unnecessarily.


In general it is risky to change regs_ever_live after reload is
complete.  Changing whether a callee saved register is live will tend
to break the assumptions made by register elimination and prologue and
epilogue threading.  I think it would be appropriate to add some
asserts to detect any case where this happens, and either fix it or
make sure it is OK.

Ian


2007-05-24  Ian Lance Taylor  <iant@google.com>

	* reload1.c (mark_home_live_1): New static function, broken out of
	mark_home_live.
	(mark_home_live): Call mark_home_live_1.
	(scan_paradoxical_subregs): Call mark_home_live_1.


Index: gcc/reload1.c
===================================================================
--- gcc/reload1.c	(revision 125036)
+++ gcc/reload1.c	(working copy)
@@ -2141,21 +2141,31 @@ alter_reg (int i, int from_reg)
     }
 }
 
-/* Mark the slots in regs_ever_live for the hard regs
-   used by pseudo-reg number REGNO.  */
+/* Mark the slots in regs_ever_live for the hard regs used by
+   pseudo-reg number REGNO, accessed in MODE.  */
 
-void
-mark_home_live (int regno)
+static void
+mark_home_live_1 (int regno, enum machine_mode mode)
 {
   int i, lim;
 
   i = reg_renumber[regno];
   if (i < 0)
     return;
-  lim = i + hard_regno_nregs[i][PSEUDO_REGNO_MODE (regno)];
+  lim = i + hard_regno_nregs[i][mode];
   while (i < lim)
     df_set_regs_ever_live(i++, true);
 }
+
+/* Mark the slots in regs_ever_live for the hard regs
+   used by pseudo-reg number REGNO.  */
+
+void
+mark_home_live (int regno)
+{
+  if (reg_renumber[regno] >= 0)
+    mark_home_live_1 (regno, PSEUDO_REGNO_MODE (regno));
+}
 \f
 /* This function handles the tracking of elimination offsets around branches.
 
@@ -3905,8 +3915,11 @@ scan_paradoxical_subregs (rtx x)
       if (REG_P (SUBREG_REG (x))
 	  && (GET_MODE_SIZE (GET_MODE (x))
 	      > reg_max_ref_width[REGNO (SUBREG_REG (x))]))
-	reg_max_ref_width[REGNO (SUBREG_REG (x))]
-	  = GET_MODE_SIZE (GET_MODE (x));
+	{
+	  reg_max_ref_width[REGNO (SUBREG_REG (x))]
+	    = GET_MODE_SIZE (GET_MODE (x));
+	  mark_home_live_1 (REGNO (SUBREG_REG (x)), GET_MODE (x));
+	}
       return;
 
     default:

Re: Dataflow miscompiles soft-fp/floatuntitf.c on x86_64

[Kenneth Zadeck]

Ian Lance Taylor wrote:
> Uros Bizjak <ubizjak@gmail.com> writes:
>
>   
>> Latest dataflow branch segfaults for:
>>
>> FAIL: gcc.dg/torture/fp-int-convert-float128-timode.c  -O0  execution test
>> FAIL: gcc.dg/torture/fp-int-convert-float128-timode.c  -O1  execution test
>> FAIL: gcc.dg/torture/fp-int-convert-float128-timode.c  -O2  execution test
>> FAIL: gcc.dg/torture/fp-int-convert-float128-timode.c  -O3
>> -fomit-frame-pointer  execution test
>> FAIL: gcc.dg/torture/fp-int-convert-float128-timode.c  -O3 -g
>> execution test
>> FAIL: gcc.dg/torture/fp-int-convert-float128-timode.c  -Os  execution test
>>
>> (The tests were run on FC6, x86_64-pc-linux-gnu).
>>
>> This is due to segfault in soft-fp/floatuntitf.c at __floatuntitf+202,
>> where %rsp is not aligned to 16 bytes for movdqa:
>>
>> 0x0000000000402c10 <__floatuntitf+192>: mov    %ax,0xfffffffffffffffe(%rsp)
>> 0x0000000000402c15 <__floatuntitf+197>: andb
>> $0x7f,0xffffffffffffffff(%rsp)
>> 0x0000000000402c1a <__floatuntitf+202>: movdqa
>> 0xfffffffffffffff0(%rsp),%xmm0   <<<
>> 0x0000000000402c20 <__floatuntitf+208>: pop    %rbx
>> 0x0000000000402c21 <__floatuntitf+209>: pop    %rbp
>> 0x0000000000402c22 <__floatuntitf+210>: retq  Uros.
>>     
>
>
> Seongbae tracked this down.  It's an interesting problem.
>
> In the .lreg dump we see this:
>
> ;; Register 79 in 5.
>
> ...
>
> (insn:HI 4 3 5 2 ../../../gccDBaseline/libgcc/../gcc/config/soft-fp/floatuntitf.c:35 (set (reg:TI 81 [ i ])
>         (subreg:TI (reg:DI 79 [ i ]) 0)) 87 {*movti_rex64} (expr_list:REG_DEAD (reg:DI 79 [ i ])
>         (nil)))
>
> Note that paradoxical subeg applied to reg 79.  Note that pseudo reg
> 79 is put in hard reg 5 (%rdi).
>
> When reload starts, it calls mark_home_live to call
> df_set_regs_ever_live for each hard register being used.  Since reg 79
> is DImode, and has been put in hard reg 5 this marks hard reg 5 as
> live (this is x86_64 so hard reg 5 is DImode).
>
> reload proceeds as usual.  At the end, reload calls
> cleanup_subreg_operands.  That function fixes up any subregs of hard
> registers to refer directly to the register in question.  In this
> case, though, it turns the paradoxical subreg into a TImode reference
> to hard reg 5, giving us this in the .greg dump:
>
> (insn:HI 4 7 5 2 ../../../gccDBaseline/libgcc/../gcc/config/soft-fp/floatuntitf.c:35 (set (reg:TI 37 r8 [orig:81 i ] [81])
>         (reg:TI 5 di [orig:79 i ] [79])) 87 {*movti_rex64} (nil))
>
> So now we have a TImode reference to hard reg 5, which means hard regs
> 5 and 6.  But when we set regs_ever_live, we only marked 5.  As it
> happens, reg 6 was not otherwise referenced in the function.  And, as
> it happens, reg 6 is %rbp, which is callee saved.
>
> All of the above happens on both mainline and dataflow branch.
>
> On mainline we simply carry on, with a reference to hard reg 6 but
> with that bit clear in regs_ever_live.  Since this is paradoxical
> subreg, hard reg 6 is never actually changed, and indeed all the
> references to it drop out as well.  So mainline is fine.
>
> Dataflow branch, however, recomputes regs_ever_live after reload is
> finished.  When it does this, it sees the TImode reference to reg 5,
> and therefore marks both reg 5 and 6 as live.
>
> But it only does this after reload is finished.  The effect is that
> the value of regs_ever_live is not the same during and after reload.
> reg 6 is clear in regs_ever_live during reload, and set after reload.
>
> This is bad.  It's bad because the x86_64 backend uses regs_ever_live
> when determining the frame layout, which is done when computing
> register elimination during reload.  And the x86_64 backend uses
> regs_ever_live again when deciding which registers to save and restore
> in the prologue and epilogue, after reload.  When the frame layout
> computation changes, it means that the stack usage changes, and the
> register elimination computations are incorrect.
>
> Fortunately the fix turns out to be relatively simple.  I have
> appended the untested patch.
>
> Note that this means that dataflow branch will generate worse code for
> this test case: it will save %rbp unnecessarily.
>
>
> In general it is risky to change regs_ever_live after reload is
> complete.  Changing whether a callee saved register is live will tend
> to break the assumptions made by register elimination and prologue and
> epilogue threading.  I think it would be appropriate to add some
> asserts to detect any case where this happens, and either fix it or
> make sure it is OK.
>
> Ian
>
>
> 2007-05-24  Ian Lance Taylor  <iant@google.com>
>
> 	* reload1.c (mark_home_live_1): New static function, broken out of
> 	mark_home_live.
> 	(mark_home_live): Call mark_home_live_1.
> 	(scan_paradoxical_subregs): Call mark_home_live_1.
>
>
> Index: gcc/reload1.c
> ===================================================================
> --- gcc/reload1.c	(revision 125036)
> +++ gcc/reload1.c	(working copy)
> @@ -2141,21 +2141,31 @@ alter_reg (int i, int from_reg)
>      }
>  }
>  
> -/* Mark the slots in regs_ever_live for the hard regs
> -   used by pseudo-reg number REGNO.  */
> +/* Mark the slots in regs_ever_live for the hard regs used by
> +   pseudo-reg number REGNO, accessed in MODE.  */
>  
> -void
> -mark_home_live (int regno)
> +static void
> +mark_home_live_1 (int regno, enum machine_mode mode)
>  {
>    int i, lim;
>  
>    i = reg_renumber[regno];
>    if (i < 0)
>      return;
> -  lim = i + hard_regno_nregs[i][PSEUDO_REGNO_MODE (regno)];
> +  lim = i + hard_regno_nregs[i][mode];
>    while (i < lim)
>      df_set_regs_ever_live(i++, true);
>  }
> +
> +/* Mark the slots in regs_ever_live for the hard regs
> +   used by pseudo-reg number REGNO.  */
> +
> +void
> +mark_home_live (int regno)
> +{
> +  if (reg_renumber[regno] >= 0)
> +    mark_home_live_1 (regno, PSEUDO_REGNO_MODE (regno));
> +}
>  \f
>  /* This function handles the tracking of elimination offsets around branches.
>  
> @@ -3905,8 +3915,11 @@ scan_paradoxical_subregs (rtx x)
>        if (REG_P (SUBREG_REG (x))
>  	  && (GET_MODE_SIZE (GET_MODE (x))
>  	      > reg_max_ref_width[REGNO (SUBREG_REG (x))]))
> -	reg_max_ref_width[REGNO (SUBREG_REG (x))]
> -	  = GET_MODE_SIZE (GET_MODE (x));
> +	{
> +	  reg_max_ref_width[REGNO (SUBREG_REG (x))]
> +	    = GET_MODE_SIZE (GET_MODE (x));
> +	  mark_home_live_1 (REGNO (SUBREG_REG (x)), GET_MODE (x));
> +	}
>        return;
>  
>      default:
>   
For all of you who help track this down, thankyou very much.

I will test this tonight and report back tomorrow.  For the short term
(until after the merge is done) i certainly do not mind saving an extra
reg.  This seems like a pretty obscure case so the cost should not be
that bad.

Seongbae and i chatted a little bit about this after iant left and in
the long run, this seems like the poster child of why regs_ever_live
should go away.  We should have a datastructure that is set by reload
and used only by prologue and eplogue generation (and is freed after
that) that communicates which registers are to be saved and in what
order and that fact only.  I.e. the smarts should all be in reload where
the decisions are made, then all that prologue and epilogue generation
do is select the insns and put them in the right place.  Doing it this
way will make the datastructure hard to overuse/overload in the future.

kenny

[dataflow]: PATCH COMMITTED Re: Dataflow miscompiles soft-fp/floatuntitf.c on x86_64

[Kenneth Zadeck]

This patch by ian taylor fixes a regression on the x86-64.   The problem
appears to be explicit on the dataflow branch and latent on the trunk.

the patch has been bootstrapped and tested on x86-64, x86-32, ppc, and
ia-64.

Kenny

Ian Lance Taylor wrote:
> Uros Bizjak <ubizjak@gmail.com> writes:
>
>   
>> Latest dataflow branch segfaults for:
>>
>> FAIL: gcc.dg/torture/fp-int-convert-float128-timode.c  -O0  execution test
>> FAIL: gcc.dg/torture/fp-int-convert-float128-timode.c  -O1  execution test
>> FAIL: gcc.dg/torture/fp-int-convert-float128-timode.c  -O2  execution test
>> FAIL: gcc.dg/torture/fp-int-convert-float128-timode.c  -O3
>> -fomit-frame-pointer  execution test
>> FAIL: gcc.dg/torture/fp-int-convert-float128-timode.c  -O3 -g
>> execution test
>> FAIL: gcc.dg/torture/fp-int-convert-float128-timode.c  -Os  execution test
>>
>> (The tests were run on FC6, x86_64-pc-linux-gnu).
>>
>> This is due to segfault in soft-fp/floatuntitf.c at __floatuntitf+202,
>> where %rsp is not aligned to 16 bytes for movdqa:
>>
>> 0x0000000000402c10 <__floatuntitf+192>: mov    %ax,0xfffffffffffffffe(%rsp)
>> 0x0000000000402c15 <__floatuntitf+197>: andb
>> $0x7f,0xffffffffffffffff(%rsp)
>> 0x0000000000402c1a <__floatuntitf+202>: movdqa
>> 0xfffffffffffffff0(%rsp),%xmm0   <<<
>> 0x0000000000402c20 <__floatuntitf+208>: pop    %rbx
>> 0x0000000000402c21 <__floatuntitf+209>: pop    %rbp
>> 0x0000000000402c22 <__floatuntitf+210>: retq  Uros.
>>     
>
>
> Seongbae tracked this down.  It's an interesting problem.
>
> In the .lreg dump we see this:
>
> ;; Register 79 in 5.
>
> ...
>
> (insn:HI 4 3 5 2 ../../../gccDBaseline/libgcc/../gcc/config/soft-fp/floatuntitf.c:35 (set (reg:TI 81 [ i ])
>         (subreg:TI (reg:DI 79 [ i ]) 0)) 87 {*movti_rex64} (expr_list:REG_DEAD (reg:DI 79 [ i ])
>         (nil)))
>
> Note that paradoxical subeg applied to reg 79.  Note that pseudo reg
> 79 is put in hard reg 5 (%rdi).
>
> When reload starts, it calls mark_home_live to call
> df_set_regs_ever_live for each hard register being used.  Since reg 79
> is DImode, and has been put in hard reg 5 this marks hard reg 5 as
> live (this is x86_64 so hard reg 5 is DImode).
>
> reload proceeds as usual.  At the end, reload calls
> cleanup_subreg_operands.  That function fixes up any subregs of hard
> registers to refer directly to the register in question.  In this
> case, though, it turns the paradoxical subreg into a TImode reference
> to hard reg 5, giving us this in the .greg dump:
>
> (insn:HI 4 7 5 2 ../../../gccDBaseline/libgcc/../gcc/config/soft-fp/floatuntitf.c:35 (set (reg:TI 37 r8 [orig:81 i ] [81])
>         (reg:TI 5 di [orig:79 i ] [79])) 87 {*movti_rex64} (nil))
>
> So now we have a TImode reference to hard reg 5, which means hard regs
> 5 and 6.  But when we set regs_ever_live, we only marked 5.  As it
> happens, reg 6 was not otherwise referenced in the function.  And, as
> it happens, reg 6 is %rbp, which is callee saved.
>
> All of the above happens on both mainline and dataflow branch.
>
> On mainline we simply carry on, with a reference to hard reg 6 but
> with that bit clear in regs_ever_live.  Since this is paradoxical
> subreg, hard reg 6 is never actually changed, and indeed all the
> references to it drop out as well.  So mainline is fine.
>
> Dataflow branch, however, recomputes regs_ever_live after reload is
> finished.  When it does this, it sees the TImode reference to reg 5,
> and therefore marks both reg 5 and 6 as live.
>
> But it only does this after reload is finished.  The effect is that
> the value of regs_ever_live is not the same during and after reload.
> reg 6 is clear in regs_ever_live during reload, and set after reload.
>
> This is bad.  It's bad because the x86_64 backend uses regs_ever_live
> when determining the frame layout, which is done when computing
> register elimination during reload.  And the x86_64 backend uses
> regs_ever_live again when deciding which registers to save and restore
> in the prologue and epilogue, after reload.  When the frame layout
> computation changes, it means that the stack usage changes, and the
> register elimination computations are incorrect.
>
> Fortunately the fix turns out to be relatively simple.  I have
> appended the untested patch.
>
> Note that this means that dataflow branch will generate worse code for
> this test case: it will save %rbp unnecessarily.
>
>
> In general it is risky to change regs_ever_live after reload is
> complete.  Changing whether a callee saved register is live will tend
> to break the assumptions made by register elimination and prologue and
> epilogue threading.  I think it would be appropriate to add some
> asserts to detect any case where this happens, and either fix it or
> make sure it is OK.
>
> Ian
>
>
> 2007-05-24  Ian Lance Taylor  <iant@google.com>
>
> 	* reload1.c (mark_home_live_1): New static function, broken out of
> 	mark_home_live.
> 	(mark_home_live): Call mark_home_live_1.
> 	(scan_paradoxical_subregs): Call mark_home_live_1.
>
>
> Index: gcc/reload1.c
> ===================================================================
> --- gcc/reload1.c	(revision 125036)
> +++ gcc/reload1.c	(working copy)
> @@ -2141,21 +2141,31 @@ alter_reg (int i, int from_reg)
>      }
>  }
>  
> -/* Mark the slots in regs_ever_live for the hard regs
> -   used by pseudo-reg number REGNO.  */
> +/* Mark the slots in regs_ever_live for the hard regs used by
> +   pseudo-reg number REGNO, accessed in MODE.  */
>  
> -void
> -mark_home_live (int regno)
> +static void
> +mark_home_live_1 (int regno, enum machine_mode mode)
>  {
>    int i, lim;
>  
>    i = reg_renumber[regno];
>    if (i < 0)
>      return;
> -  lim = i + hard_regno_nregs[i][PSEUDO_REGNO_MODE (regno)];
> +  lim = i + hard_regno_nregs[i][mode];
>    while (i < lim)
>      df_set_regs_ever_live(i++, true);
>  }
> +
> +/* Mark the slots in regs_ever_live for the hard regs
> +   used by pseudo-reg number REGNO.  */
> +
> +void
> +mark_home_live (int regno)
> +{
> +  if (reg_renumber[regno] >= 0)
> +    mark_home_live_1 (regno, PSEUDO_REGNO_MODE (regno));
> +}
>  \f
>  /* This function handles the tracking of elimination offsets around branches.
>  
> @@ -3905,8 +3915,11 @@ scan_paradoxical_subregs (rtx x)
>        if (REG_P (SUBREG_REG (x))
>  	  && (GET_MODE_SIZE (GET_MODE (x))
>  	      > reg_max_ref_width[REGNO (SUBREG_REG (x))]))
> -	reg_max_ref_width[REGNO (SUBREG_REG (x))]
> -	  = GET_MODE_SIZE (GET_MODE (x));
> +	{
> +	  reg_max_ref_width[REGNO (SUBREG_REG (x))]
> +	    = GET_MODE_SIZE (GET_MODE (x));
> +	  mark_home_live_1 (REGNO (SUBREG_REG (x)), GET_MODE (x));
> +	}
>        return;
>  
>      default:
>