[Bug target/31478] New: Typo in sse2_umulv2siv2di3 pattern

[Bug target/31478] New: Typo in sse2_umulv2siv2di3 pattern

[hjl at lucon dot org]

There are

(define_insn "sse2_umulv2siv2di3"
  [(set (match_operand:V2DI 0 "register_operand" "=x")
        (mult:V2DI
          (zero_extend:V2DI
            (vec_select:V2SI
              (match_operand:V4SI 1 "nonimmediate_operand" "%0")
              (parallel [(const_int 0) (const_int 2)])))
          (zero_extend:V2DI 
            (vec_select:V2SI
              (match_operand:V4SI 2 "nonimmediate_operand" "xm")
              (parallel [(const_int 0) (const_int 2)])))))]
  "TARGET_SSE2 && ix86_binary_operator_ok (MULT, V8HImode, operands)"
  "pmuludq\t{%2, %0|%0, %2}"
  [(set_attr "type" "sseimul")
   (set_attr "mode" "TI")])

I have 2 questions:

1. Is ix86_binary_operator_ok needed here?
2. If yes, why is V8HImode used?


-- 
           Summary: Typo in sse2_umulv2siv2di3 pattern
           Product: gcc
           Version: 4.3.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: hjl at lucon dot org
GCC target triplet: x86_64-unknown-linux-gnu


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=31478

[Bug target/31478] Typo in sse2_umulv2siv2di3 pattern

[ubizjak at gmail dot com]

------- Comment #1 from ubizjak at gmail dot com  2007-04-05 07:15 -------
(In reply to comment #0)

> 1. Is ix86_binary_operator_ok needed here?

Yes, it prevents expander and combiner to create two mem operands (please note
that reload can also resolve this case by itself, but some CSE opportunities
can be missed).

> 2. If yes, why is V8HImode used?

Actually, mode is ignored for nonimmediate operands. For consistency, V4SI
could be put here - the patch for this change is preapproved.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=31478

[Bug target/31478] Typo in sse2_umulv2siv2di3 pattern

[hjl at lucon dot org]

------- Comment #2 from hjl at lucon dot org  2007-04-05 07:35 -------
(In reply to comment #1)
> (In reply to comment #0)
> 
> > 1. Is ix86_binary_operator_ok needed here?
> 
> Yes, it prevents expander and combiner to create two mem operands (please note
> that reload can also resolve this case by itself, but some CSE opportunities
> can be missed).
> 

There is no corresponding define_expand for this pattern. Many define_insn
patterns without define_expand don't call ix86_binary_operator_ok. Will that
be a problem?


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=31478

[Bug target/31478] Typo in sse2_umulv2siv2di3 pattern

[ubizjak at gmail dot com]

------- Comment #3 from ubizjak at gmail dot com  2007-04-05 08:48 -------
> There is no corresponding define_expand for this pattern. Many define_insn
> patterns without define_expand don't call ix86_binary_operator_ok. Will that
> be a problem?

Those are sse builtins and are expanded via ix86_expand_binop_builtin(). This
function fixes all operands to satisfy ix86_binary_operator_ok().

Regarding call to ix86_binary_operator_ok - this check is needed only when both
input operands are nonimmediate_operand (and output operand is
register_operand). There is indeed a pattern without this check - sse2_pmaddwd.
Due to %, it is commutative, so check for PLUS of V8HI mode would be OK.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=31478

[Bug target/31478] Typo in sse2_umulv2siv2di3 pattern

[ubizjak at gmail dot com]

------- Comment #4 from ubizjak at gmail dot com  2007-04-05 09:13 -------
(In reply to comment #3)

> There is indeed a pattern without this check - sse2_pmaddwd.
> Due to %, it is commutative, so check for PLUS of V8HI mode would be OK.

Please also change operands 1 and 2 of sdot_prodv8hi expander to
register_operand to avoid further suprises.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=31478

[Bug target/31478] Typo in sse2_umulv2siv2di3 pattern

[hjl at lucon dot org]

------- Comment #5 from hjl at lucon dot org  2007-04-05 15:12 -------
(In reply to comment #3)
> > There is no corresponding define_expand for this pattern. Many define_insn
> > patterns without define_expand don't call ix86_binary_operator_ok. Will that
> > be a problem?
> 
> Those are sse builtins and are expanded via ix86_expand_binop_builtin(). This
> function fixes all operands to satisfy ix86_binary_operator_ok().
> 

So anothe word is those patterns are used by ix86_expand_binop_builtin()
and won't be generated automatically. Will be "sse2_umulv2siv2di3"
generated automatically? If yes, what makes it different from other
patterns used by ix86_expand_binop_builtin() only?


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=31478

[Bug target/31478] Typo in sse2_umulv2siv2di3 pattern

[ubizjak at gmail dot com]

------- Comment #6 from ubizjak at gmail dot com  2007-04-05 16:48 -------
(In reply to comment #5)

> So anothe word is those patterns are used by ix86_expand_binop_builtin()
> and won't be generated automatically. Will be "sse2_umulv2siv2di3"
> generated automatically? If yes, what makes it different from other
> patterns used by ix86_expand_binop_builtin() only?

No, they won't be generated automatically, because they don't have the name
that is recognized by generic expander. IOW, they are not named insn pattern.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=31478

[Bug target/31478] Typo in sse2_umulv2siv2di3 pattern

[hjl at lucon dot org]

------- Comment #7 from hjl at lucon dot org  2007-04-05 16:54 -------
(In reply to comment #6)
> (In reply to comment #5)
> 
> > So anothe word is those patterns are used by ix86_expand_binop_builtin()
> > and won't be generated automatically. Will be "sse2_umulv2siv2di3"
> > generated automatically? If yes, what makes it different from other
> > patterns used by ix86_expand_binop_builtin() only?
> 
> No, they won't be generated automatically, because they don't have the name
> that is recognized by generic expander. IOW, they are not named insn pattern.
> 

Since sse2_umulv2siv2di3 is used by ix86_expand_binop_builtin() and sse.md
only:

[hjl@gnu-2 i386]$ grep sse2_umulv2siv2di3 *.c *.md
i386.c:  { MASK_SSE2, CODE_FOR_sse2_umulv2siv2di3, 0, IX86_BUILTIN_PMULUDQ128,
0, 0 },
sse.md:(define_insn "sse2_umulv2siv2di3"
sse.md:  emit_insn (gen_sse2_umulv2siv2di3 (gen_lowpart (V2DImode, t1), op1,
op2));
sse.md:  emit_insn (gen_sse2_umulv2siv2di3 (gen_lowpart (V2DImode, t4), t2,
t3));
sse.md:  emit_insn (gen_sse2_umulv2siv2di3 (t1, gen_lowpart (V4SImode, op1),
sse.md:  emit_insn (gen_sse2_umulv2siv2di3 (t4, gen_lowpart (V4SImode, op1),
sse.md:  emit_insn (gen_sse2_umulv2siv2di3 (t5, gen_lowpart (V4SImode, op2),
sse.md:  emit_insn (gen_sse2_umulv2siv2di3 (operands[0], t1, t2));
sse.md:  emit_insn (gen_sse2_umulv2siv2di3 (operands[0], t1, t2));
sse.md:  emit_insn (gen_sse2_umulv2siv2di3 (operands[0], t1, t2));
sse.md:  emit_insn (gen_sse2_umulv2siv2di3 (operands[0], t1, t2));
sse.md:  emit_insn (gen_sse2_umulv2siv2di3 (t1, operands[1], operands[2]));
sse.md:  emit_insn (gen_sse2_umulv2siv2di3 (t4, t2, t3));

is ix86_binary_operator_ok in sse2_umulv2siv2di3 not used at all and really
optional?


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=31478

[Bug target/31478] Typo in sse2_umulv2siv2di3 pattern

[hjl at lucon dot org]

------- Comment #8 from hjl at lucon dot org  2007-04-05 17:54 -------
(In reply to comment #4)
> (In reply to comment #3)
> 
> > There is indeed a pattern without this check - sse2_pmaddwd.
> > Due to %, it is commutative, so check for PLUS of V8HI mode would be OK.
> 
> Please also change operands 1 and 2 of sdot_prodv8hi expander to
> register_operand to avoid further suprises.
> 

I am not sure if there is an issue since op0 of sse2_pmaddwd must be
register and it must be the same as op1 or op2.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=31478

[Bug target/31478] Typo in sse2_umulv2siv2di3 pattern

[ubizjak at gmail dot com]

------- Comment #9 from ubizjak at gmail dot com  2007-04-05 19:50 -------
(In reply to comment #8)

> > Please also change operands 1 and 2 of sdot_prodv8hi expander to
> > register_operand to avoid further suprises.
> > 
> 
> I am not sure if there is an issue since op0 of sse2_pmaddwd must be
> register and it must be the same as op1 or op2.

But only after reload. Nothing prevents gcc to generate two MEM input operands.
Although reload will fix this by putting one of input operands into register
(and match it with output operand), but this will be too late for any
pre-reload pass to make any optimization on inputs.

> is ix86_binary_operator_ok in sse2_umulv2siv2di3 not used at all and really
> optional?

No, it is not optional. V8HImode argument is optional, since we don't allow
immediate operands. Please track code flow through ix86_b_o_ok() and you will
see.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=31478

[Bug target/31478] Typo in sse2_umulv2siv2di3 pattern

[hjl at lucon dot org]

------- Comment #10 from hjl at lucon dot org  2007-04-05 20:05 -------
(In reply to comment #9)
> (In reply to comment #8)
> 
> > > Please also change operands 1 and 2 of sdot_prodv8hi expander to
> > > register_operand to avoid further suprises.
> > > 
> > 
> > I am not sure if there is an issue since op0 of sse2_pmaddwd must be
> > register and it must be the same as op1 or op2.
> 
> But only after reload. Nothing prevents gcc to generate two MEM input operands.
> Although reload will fix this by putting one of input operands into register
> (and match it with output operand), but this will be too late for any
> pre-reload pass to make any optimization on inputs.
> 

Will something like

--- sse.md.pmaddwd      2007-04-04 17:44:43.000000000 -0700
+++ sse.md      2007-04-05 12:02:28.000000000 -0700
@@ -3044,7 +3044,7 @@
    (match_operand:V8HI 1 "nonimmediate_operand" "")
    (match_operand:V8HI 2 "nonimmediate_operand" "")
    (match_operand:V4SI 3 "register_operand" "")]
-  "TARGET_SSE2"
+  "TARGET_SSE && !(MEM_P (operands[1]) && MEM_P (operands[2]))"
 {
   rtx t = gen_reg_rtx (V4SImode);
   emit_insn (gen_sse2_pmaddwd (t, operands[1], operands[2]));

work?


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=31478

[Bug target/31478] Typo in sse2_umulv2siv2di3 pattern

[hjl at lucon dot org]

------- Comment #11 from hjl at lucon dot org  2007-04-05 20:06 -------
(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > 
> > > > Please also change operands 1 and 2 of sdot_prodv8hi expander to
> > > > register_operand to avoid further suprises.
> > > > 
> > > 
> > > I am not sure if there is an issue since op0 of sse2_pmaddwd must be
> > > register and it must be the same as op1 or op2.
> > 
> > But only after reload. Nothing prevents gcc to generate two MEM input operands.
> > Although reload will fix this by putting one of input operands into register
> > (and match it with output operand), but this will be too late for any
> > pre-reload pass to make any optimization on inputs.
> > 
> 
> Will something like
> 
> --- sse.md.pmaddwd      2007-04-04 17:44:43.000000000 -0700
> +++ sse.md      2007-04-05 12:02:28.000000000 -0700
> @@ -3044,7 +3044,7 @@
>     (match_operand:V8HI 1 "nonimmediate_operand" "")
>     (match_operand:V8HI 2 "nonimmediate_operand" "")
>     (match_operand:V4SI 3 "register_operand" "")]
> -  "TARGET_SSE2"
> +  "TARGET_SSE && !(MEM_P (operands[1]) && MEM_P (operands[2]))"
>  {
>    rtx t = gen_reg_rtx (V4SImode);
>    emit_insn (gen_sse2_pmaddwd (t, operands[1], operands[2]));
> 
> work?
> 

It doesn't work.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=31478

[Bug target/31478] Typo in sse2_umulv2siv2di3 pattern

[hjl at gcc dot gnu dot org]

------- Comment #12 from hjl at gcc dot gnu dot org  2007-04-05 22:16 -------
Subject: Bug 31478

Author: hjl
Date: Thu Apr  5 22:16:37 2007
New Revision: 123527

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=123527
Log:
2007-04-05  Uros Bizjak  <ubizjak@gmail.com>
            H.J. Lu  <hongjiu.lu@intel.com>

        PR target/31478
        * config/i386/sse.md (sse2_umulv2siv2di3): Use V4SImode instead
        of V8HImode when calling ix86_binary_operator_ok.
        (sse2_pmaddwd): Call ix86_binary_operator_ok.
        (sdot_prodv8hi): Operands 1 and 2 must be register.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/i386/sse.md


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=31478

[Bug target/31478] Typos in sse2_umulv2siv2di3, sse2_pmaddwd, sdot_prodv8hi

[rguenth at gcc dot gnu dot org]

------- Comment #13 from rguenth at gcc dot gnu dot org  2008-03-27 22:13 -------
Fixed.


-- 

rguenth at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
      Known to fail|                            |4.1.3 4.2.3
      Known to work|                            |4.3.0
         Resolution|                            |FIXED
            Summary|[4.1/4.2]: Typos in         |Typos in sse2_umulv2siv2di3,
                   |sse2_umulv2siv2di3,         |sse2_pmaddwd, sdot_prodv8hi
                   |sse2_pmaddwd, sdot_prodv8hi |
   Target Milestone|---                         |4.3.0


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=31478