Patchwork PATCH: Fix split_stack_return

login
register
mail settings
Submitter H.J. Lu
Date Oct. 26, 2010, 9:17 p.m.
Message ID <20101026211731.GA3834@intel.com>
Download mbox | patch
Permalink /patch/69292/
State New
Headers show

Comments

H.J. Lu - Oct. 26, 2010, 9:17 p.m.
Hi Ian,

I am checking in this patch since wrong instruction is generated for
my vzeroupper work.


H.J.
----
2010-10-26  H.J. Lu  <hongjiu.lu@intel.com>

	* config/i386/i386.md (split_stack_return): Replace
	unspec_volatile with unspec.
Andrew Pinski - Oct. 26, 2010, 9:19 p.m.
On Tue, Oct 26, 2010 at 2:17 PM, H.J. Lu <hongjiu.lu@intel.com> wrote:
> Hi Ian,
>
> I am checking in this patch since wrong instruction is generated for
> my vzeroupper work.

How is that obvious?  Why does it generate the wrong instruction?

Thanks,
Andrew Pinski
H.J. Lu - Oct. 26, 2010, 9:26 p.m.
On Tue, Oct 26, 2010 at 2:19 PM, Andrew Pinski <pinskia@gmail.com> wrote:
> On Tue, Oct 26, 2010 at 2:17 PM, H.J. Lu <hongjiu.lu@intel.com> wrote:
>> Hi Ian,
>>
>> I am checking in this patch since wrong instruction is generated for
>> my vzeroupper work.
>
> How is that obvious?  Why does it generate the wrong instruction?
>
>


(define_insn "split_stack_return"
  [(unspec_volatile [(match_operand:SI 0 "const_int_operand" "")]
                    UNSPEC_STACK_CHECK)]


vs.

(define_insn "avx_vzeroupper"
  [(unspec_volatile [(match_operand 0 "const_int_operand" "")]
                    UNSPECV_VZEROUPPER)]

I generate avx_zeroupper.  But it matches split_stack_return
since (int) UNSPEC_STACK_CHECK == (int) UNSPECV_VZEROUPPER.
Andrew Pinski - Oct. 26, 2010, 9:27 p.m.
On Tue, Oct 26, 2010 at 2:26 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>
>
> vs.
>
> (define_insn "avx_vzeroupper"
>  [(unspec_volatile [(match_operand 0 "const_int_operand" "")]
>                    UNSPECV_VZEROUPPER)]
>
> I generate avx_zeroupper.  But it matches split_stack_return
> since (int) UNSPEC_STACK_CHECK == (int) UNSPECV_VZEROUPPER.

Why are they equal?  That is the bug really.

-- Pinski
H.J. Lu - Oct. 26, 2010, 9:37 p.m.
On Tue, Oct 26, 2010 at 2:27 PM, Andrew Pinski <pinskia@gmail.com> wrote:
> On Tue, Oct 26, 2010 at 2:26 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>
>>
>> vs.
>>
>> (define_insn "avx_vzeroupper"
>>  [(unspec_volatile [(match_operand 0 "const_int_operand" "")]
>>                    UNSPECV_VZEROUPPER)]
>>
>> I generate avx_zeroupper.  But it matches split_stack_return
>> since (int) UNSPEC_STACK_CHECK == (int) UNSPECV_VZEROUPPER.
>
> Why are they equal?  That is the bug really.
>

One is UNSPEC and the other is USPECV. They are different.
Andrew Pinski - Oct. 26, 2010, 9:39 p.m.
On Tue, Oct 26, 2010 at 2:37 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> One is UNSPEC and the other is USPECV. They are different.

Except you just changed that.  Both were unspec_volatile before.  So
why not change the value rather than unspec vs unspec_volatile?

-- Pinski
H.J. Lu - Oct. 26, 2010, 10:10 p.m.
On Tue, Oct 26, 2010 at 2:39 PM, Andrew Pinski <pinskia@gmail.com> wrote:
> On Tue, Oct 26, 2010 at 2:37 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> One is UNSPEC and the other is USPECV. They are different.
>
> Except you just changed that.  Both were unspec_volatile before.  So
> why not change the value rather than unspec vs unspec_volatile?
>

I don't want to change

(define_expand "split_stack_space_check"
  [(set (pc) (if_then_else
              (ltu (minus (reg SP_REG)
                          (match_operand 0 "register_operand" ""))
                   (unspec [(const_int 0)] UNSPEC_STACK_CHECK))
              (label_ref (match_operand 1 "" ""))
              (pc)))]
  ""
{
  rtx reg, size, limit;

  reg = gen_reg_rtx (Pmode);
  size = force_reg (Pmode, operands[0]);
  emit_insn (gen_sub3_insn (reg, stack_pointer_rtx, size));
  limit = gen_rtx_UNSPEC (Pmode, gen_rtvec (1, const0_rtx),
                          UNSPEC_STACK_CHECK);
  limit = gen_rtx_MEM (Pmode, gen_rtx_CONST (Pmode, limit));
  ix86_expand_branch (GEU, reg, limit, operands[1]);


It uses unspec, not unspec_volatile.
Ian Taylor - Oct. 26, 2010, 10:28 p.m.
On Tue, Oct 26, 2010 at 2:17 PM, H.J. Lu <hongjiu.lu@intel.com> wrote:

> 2010-10-26  H.J. Lu  <hongjiu.lu@intel.com>
>
>        * config/i386/i386.md (split_stack_return): Replace
>        unspec_volatile with unspec.

But that is clearly incorrect.  This patch is not obvious and is not
approved.  Please do not commit it.

Ian
Ian Taylor - Oct. 26, 2010, 10:40 p.m.
On Tue, Oct 26, 2010 at 3:10 PM, H.J. Lu <hjl.tools@gmail.com> wrote:

> I don't want to change

> It uses unspec, not unspec_volatile.

Right: you can't change either the unspec or the unspec_volatile.
Both are correct.

What is incorrect is that I used the same value for both.

Ian
H.J. Lu - Oct. 26, 2010, 10:49 p.m.
On Tue, Oct 26, 2010 at 3:40 PM, Ian Lance Taylor <iant@google.com> wrote:
> On Tue, Oct 26, 2010 at 3:10 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>
>> I don't want to change
>
>> It uses unspec, not unspec_volatile.
>
> Right: you can't change either the unspec or the unspec_volatile.
> Both are correct.
>
> What is incorrect is that I used the same value for both.
>
>

Please fix my checkin.

Thanks.
Ian Taylor - Oct. 27, 2010, 1:46 a.m.
"H.J. Lu" <hjl.tools@gmail.com> writes:

> On Tue, Oct 26, 2010 at 3:40 PM, Ian Lance Taylor <iant@google.com> wrote:
>> On Tue, Oct 26, 2010 at 3:10 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>
>>> I don't want to change
>>
>>> It uses unspec, not unspec_volatile.
>>
>> Right: you can't change either the unspec or the unspec_volatile.
>> Both are correct.
>>
>> What is incorrect is that I used the same value for both.
>>
>>
>
> Please fix my checkin.

First: a process issue: your patch was definitely not obvious.  You
should not have committed it.  You told me that you had found a problem,
and I agreed, but we didn't discuss that patch.  When an incorrect patch
is committed, for whatever reason, the procedure should be to
immediately revert it, and then work out the correct patch.  I will
revert your patch for you if you like.

Second: do you have a test case?  I don't see one in the svn commit.  It
would be much easier for me to write the correct patch if I had a
failing test case, even if only to look at the assembler code.

Ian
H.J. Lu - Oct. 27, 2010, 2:15 a.m.
On Tue, Oct 26, 2010 at 6:46 PM, Ian Lance Taylor <iant@google.com> wrote:
> "H.J. Lu" <hjl.tools@gmail.com> writes:
>
>> On Tue, Oct 26, 2010 at 3:40 PM, Ian Lance Taylor <iant@google.com> wrote:
>>> On Tue, Oct 26, 2010 at 3:10 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>
>>>> I don't want to change
>>>
>>>> It uses unspec, not unspec_volatile.
>>>
>>> Right: you can't change either the unspec or the unspec_volatile.
>>> Both are correct.
>>>
>>> What is incorrect is that I used the same value for both.
>>>
>>>
>>
>> Please fix my checkin.
>
> First: a process issue: your patch was definitely not obvious.  You
> should not have committed it.  You told me that you had found a problem,
> and I agreed, but we didn't discuss that patch.  When an incorrect patch
> is committed, for whatever reason, the procedure should be to
> immediately revert it, and then work out the correct patch.  I will
> revert your patch for you if you like.
>
> Second: do you have a test case?  I don't see one in the svn commit.  It
> would be much easier for me to write the correct patch if I had a
> failing test case, even if only to look at the assembler code.
>

I found the problem when I am working on my vzeroupper change.
The bad "split_stack_return" pattern makes it impossible for me
to work on vzeroupper since gcc kept generating "ret $2"
for my vzeroupper pattern. The split_stack_return issue wasted
my time and blocked my vzeroupper change, which I want to finish during
gcc summit. That is why I committed my change.

Sorry for the trouble I caused.

Patch

diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
index 2eb2a72..cfd3f65 100644
--- a/gcc/config/i386/i386.md
+++ b/gcc/config/i386/i386.md
@@ -11752,8 +11751,8 @@ 
 ;; In order to support the call/return predictor, we use a return
 ;; instruction which the middle-end doesn't see.
 (define_insn "split_stack_return"
-  [(unspec_volatile [(match_operand:SI 0 "const_int_operand" "")]
-		    UNSPEC_STACK_CHECK)]
+  [(unspec [(match_operand:SI 0 "const_int_operand" "")]
+	    UNSPEC_STACK_CHECK)]
   ""
 {
   if (operands[0] == const0_rtx)