| Message ID | 20101026211731.GA3834@intel.com |
|---|---|
| State | New |
| Headers | show |
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
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.
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
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.
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
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.
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
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
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.
"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
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.
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)