[PATCH][i386][3/3] PR target/84164: Make *cmpqi_ext_<n> patterns accept more zero

public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed

* [PATCH][i386][3/3] PR target/84164: Make *cmpqi_ext_<n> patterns accept more zero_extract modes
@ 2018-02-08 17:11 Kyrill Tkachov
  2018-02-08 22:54 ` Uros Bizjak
  0 siblings, 1 reply; 7+ messages in thread
From: Kyrill Tkachov @ 2018-02-08 17:11 UTC (permalink / raw)
  To: gcc-patches; +Cc: Uros Bizjak

[-- Attachment #1: Type: text/plain, Size: 2412 bytes --]

Hi all,

This patch fixes some fallout in the i386 testsuite that occurs after the simplification in patch [1/3] [1].
The gcc.target/i386/extract-2.c FAILs because it expects to match:
(set (reg:CC 17 flags)
     (compare:CC (subreg:QI (zero_extract:SI (reg:HI 98)
                 (const_int 8 [0x8])
                 (const_int 8 [0x8])) 0)
         (const_int 4 [0x4])))

which is the *cmpqi_ext_2 pattern in i386.md but with the new simplification the combine/simplify-rtx
machinery produces:
(set (reg:CC 17 flags)
     (compare:CC (subreg:QI (zero_extract:HI (reg:HI 98)
                 (const_int 8 [0x8])
                 (const_int 8 [0x8])) 0)
         (const_int 4 [0x4])))

Notice that the zero_extract now has HImode like the register source rather than SImode.
The existing *cmpqi_ext_<n> patterns however explicitly demand an SImode on the zero_extract.
I'm not overly familiar with the i386 port but I think that's too restrictive.
The RTL documentation says:
For (zero_extract:m loc size pos) "The mode m is the same as the mode that would be used for loc if it were a register."
I'm not sure if that means that the mode of the zero_extract and the source register must always match (as is the
case after patch [1/3]) but in any case it shouldn't matter semantically since we're taking a QImode subreg of the whole
thing anyway.

So the proposed solution in this patch is to allow HI, SI and DImode zero_extracts in these patterns as these are the
modes that the ext_register_operand predicate accepts, so that the patterns can match the new form above.

With this patch the aforementioned test passes again and bootstrap and testing on x86_64-unknown-linux-gnu shows
no regressions.

Is this ok for trunk if the first patch is accepted?

Thanks,
Kyrill

[1] https://gcc.gnu.org/ml/gcc-patches/2018-02/msg00443.html

2018-02-07  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR target/84164
     * config/i386/i386.md (*cmpqi_ext_1): Rename to...
     (*cmpqi<mode>_ext_1): ... This.  Use SWI248 mode iterator
     for zero_extract.
     (*cmpqi_ext_2): Rename to...
     (*cmpqi<mode>_ext_2): ... This.  Use SWI248 mode iterator
     for zero_extract.
     (*cmpqi_ext_3): Rename to...
     (*cmpqi<mode>_ext_3): ... This.  Use SWI248 mode iterator
     for zero_extract.
     (*cmpqi_ext_4): Rename to...
     (*cmpqi<mode>_ext_4): ... This.  Use SWI248 mode iterator
     for zero_extract.

[-- Attachment #2: i386-modes.patch --]
[-- Type: text/x-patch, Size: 1991 bytes --]

diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
index a4832bf696f321e8ee5aad71fa946ca198d9d689..ced9a3e823ae6c4586be510a782d354f4d364daa 100644
--- a/gcc/config/i386/i386.md
+++ b/gcc/config/i386/i386.md
@@ -1328,12 +1328,12 @@ (define_insn "*cmp<mode>_minus_1"
   [(set_attr "type" "icmp")
    (set_attr "mode" "<MODE>")])
 
-(define_insn "*cmpqi_ext_1"
+(define_insn "*cmpqi<mode>_ext_1"
   [(set (reg FLAGS_REG)
 	(compare
 	  (match_operand:QI 0 "nonimmediate_operand" "QBc,m")
 	  (subreg:QI
-	    (zero_extract:SI
+	    (zero_extract:SWI248
 	      (match_operand 1 "ext_register_operand" "Q,Q")
 	      (const_int 8)
 	      (const_int 8)) 0)))]
@@ -1343,11 +1343,11 @@ (define_insn "*cmpqi_ext_1"
    (set_attr "type" "icmp")
    (set_attr "mode" "QI")])
 
-(define_insn "*cmpqi_ext_2"
+(define_insn "*cmpqi<mode>_ext_2"
   [(set (reg FLAGS_REG)
 	(compare
 	  (subreg:QI
-	    (zero_extract:SI
+	    (zero_extract:SWI248
 	      (match_operand 0 "ext_register_operand" "Q")
 	      (const_int 8)
 	      (const_int 8)) 0)
@@ -1368,11 +1368,11 @@ (define_expand "cmpqi_ext_3"
 	      (const_int 8)) 0)
 	  (match_operand:QI 1 "const_int_operand")))])
 
-(define_insn "*cmpqi_ext_3"
+(define_insn "*cmpqi<mode>_ext_3"
   [(set (reg FLAGS_REG)
 	(compare
 	  (subreg:QI
-	    (zero_extract:SI
+	    (zero_extract:SWI248
 	      (match_operand 0 "ext_register_operand" "Q,Q")
 	      (const_int 8)
 	      (const_int 8)) 0)
@@ -1383,16 +1383,16 @@ (define_insn "*cmpqi_ext_3"
    (set_attr "type" "icmp")
    (set_attr "mode" "QI")])
 
-(define_insn "*cmpqi_ext_4"
+(define_insn "*cmpqi<mode>_ext_4"
   [(set (reg FLAGS_REG)
 	(compare
 	  (subreg:QI
-	    (zero_extract:SI
+	    (zero_extract:SWI248
 	      (match_operand 0 "ext_register_operand" "Q")
 	      (const_int 8)
 	      (const_int 8)) 0)
 	  (subreg:QI
-	    (zero_extract:SI
+	    (zero_extract:SWI248
 	      (match_operand 1 "ext_register_operand" "Q")
 	      (const_int 8)
 	      (const_int 8)) 0)))]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH][i386][3/3] PR target/84164: Make *cmpqi_ext_<n> patterns accept more zero_extract modes
  2018-02-08 17:11 [PATCH][i386][3/3] PR target/84164: Make *cmpqi_ext_<n> patterns accept more zero_extract modes Kyrill Tkachov
@ 2018-02-08 22:54 ` Uros Bizjak
  2018-02-09 14:50   ` Kyrill Tkachov
  0 siblings, 1 reply; 7+ messages in thread
From: Uros Bizjak @ 2018-02-08 22:54 UTC (permalink / raw)
  To: Kyrill Tkachov; +Cc: gcc-patches

On Thu, Feb 8, 2018 at 6:11 PM, Kyrill  Tkachov
<kyrylo.tkachov@foss.arm.com> wrote:
> Hi all,
>
> This patch fixes some fallout in the i386 testsuite that occurs after the
> simplification in patch [1/3] [1].
> The gcc.target/i386/extract-2.c FAILs because it expects to match:
> (set (reg:CC 17 flags)
>     (compare:CC (subreg:QI (zero_extract:SI (reg:HI 98)
>                 (const_int 8 [0x8])
>                 (const_int 8 [0x8])) 0)
>         (const_int 4 [0x4])))
>
> which is the *cmpqi_ext_2 pattern in i386.md but with the new simplification
> the combine/simplify-rtx
> machinery produces:
> (set (reg:CC 17 flags)
>     (compare:CC (subreg:QI (zero_extract:HI (reg:HI 98)
>                 (const_int 8 [0x8])
>                 (const_int 8 [0x8])) 0)
>         (const_int 4 [0x4])))
>
> Notice that the zero_extract now has HImode like the register source rather
> than SImode.
> The existing *cmpqi_ext_<n> patterns however explicitly demand an SImode on
> the zero_extract.
> I'm not overly familiar with the i386 port but I think that's too
> restrictive.
> The RTL documentation says:
> For (zero_extract:m loc size pos) "The mode m is the same as the mode that
> would be used for loc if it were a register."
> I'm not sure if that means that the mode of the zero_extract and the source
> register must always match (as is the
> case after patch [1/3]) but in any case it shouldn't matter semantically
> since we're taking a QImode subreg of the whole
> thing anyway.
>
> So the proposed solution in this patch is to allow HI, SI and DImode
> zero_extracts in these patterns as these are the
> modes that the ext_register_operand predicate accepts, so that the patterns
> can match the new form above.
>
> With this patch the aforementioned test passes again and bootstrap and
> testing on x86_64-unknown-linux-gnu shows
> no regressions.
>
> Is this ok for trunk if the first patch is accepted?

Huh, there are many other zero-extract patterns besides cmpqi_ext_*
with QImode subreg of SImode zero_extract in i386.md, used to access
high QImode register of HImode pair. A quick grep shows these that
have _ext_ in their name:

(define_insn "*cmpqi_ext_1"
(define_insn "*cmpqi_ext_2"
(define_expand "cmpqi_ext_3"
(define_insn "*cmpqi_ext_3"
(define_insn "*cmpqi_ext_4"
(define_insn "addqi_ext_1"
(define_insn "*addqi_ext_2"
(define_expand "testqi_ext_1_ccno"
(define_insn "*testqi_ext_1"
(define_insn "*testqi_ext_2"
(define_insn_and_split "*testqi_ext_3"
(define_insn "andqi_ext_1"
(define_insn "*andqi_ext_1_cc"
(define_insn "*andqi_ext_2"
(define_insn "*<code>qi_ext_1"
(define_insn "*<code>qi_ext_2"
(define_expand "xorqi_ext_1_cc"
(define_insn "*xorqi_ext_1_cc"

There are also relevant splitters and peephole2 patterns.

IIRC, SImode zero_extract was enough to catch all high-register uses.
There will be a pattern explosion if we want to handle all other
integer modes here. However, I'm not a RTL expert, so someone will
have to say what is the correct RTX form here.

Uros.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH][i386][3/3] PR target/84164: Make *cmpqi_ext_<n> patterns accept more zero_extract modes
  2018-02-08 22:54 ` Uros Bizjak
@ 2018-02-09 14:50   ` Kyrill Tkachov
  2018-02-13 16:46     ` Jeff Law
  0 siblings, 1 reply; 7+ messages in thread
From: Kyrill Tkachov @ 2018-02-09 14:50 UTC (permalink / raw)
  To: Uros Bizjak; +Cc: gcc-patches, Jeff Law, richard.sandiford

Hi Uros,

On 08/02/18 22:54, Uros Bizjak wrote:
> On Thu, Feb 8, 2018 at 6:11 PM, Kyrill  Tkachov
> <kyrylo.tkachov@foss.arm.com> wrote:
>> Hi all,
>>
>> This patch fixes some fallout in the i386 testsuite that occurs after the
>> simplification in patch [1/3] [1].
>> The gcc.target/i386/extract-2.c FAILs because it expects to match:
>> (set (reg:CC 17 flags)
>>      (compare:CC (subreg:QI (zero_extract:SI (reg:HI 98)
>>                  (const_int 8 [0x8])
>>                  (const_int 8 [0x8])) 0)
>>          (const_int 4 [0x4])))
>>
>> which is the *cmpqi_ext_2 pattern in i386.md but with the new simplification
>> the combine/simplify-rtx
>> machinery produces:
>> (set (reg:CC 17 flags)
>>      (compare:CC (subreg:QI (zero_extract:HI (reg:HI 98)
>>                  (const_int 8 [0x8])
>>                  (const_int 8 [0x8])) 0)
>>          (const_int 4 [0x4])))
>>
>> Notice that the zero_extract now has HImode like the register source rather
>> than SImode.
>> The existing *cmpqi_ext_<n> patterns however explicitly demand an SImode on
>> the zero_extract.
>> I'm not overly familiar with the i386 port but I think that's too
>> restrictive.
>> The RTL documentation says:
>> For (zero_extract:m loc size pos) "The mode m is the same as the mode that
>> would be used for loc if it were a register."
>> I'm not sure if that means that the mode of the zero_extract and the source
>> register must always match (as is the
>> case after patch [1/3]) but in any case it shouldn't matter semantically
>> since we're taking a QImode subreg of the whole
>> thing anyway.
>>
>> So the proposed solution in this patch is to allow HI, SI and DImode
>> zero_extracts in these patterns as these are the
>> modes that the ext_register_operand predicate accepts, so that the patterns
>> can match the new form above.
>>
>> With this patch the aforementioned test passes again and bootstrap and
>> testing on x86_64-unknown-linux-gnu shows
>> no regressions.
>>
>> Is this ok for trunk if the first patch is accepted?
> Huh, there are many other zero-extract patterns besides cmpqi_ext_*
> with QImode subreg of SImode zero_extract in i386.md, used to access
> high QImode register of HImode pair. A quick grep shows these that
> have _ext_ in their name:
>
> (define_insn "*cmpqi_ext_1"
> (define_insn "*cmpqi_ext_2"
> (define_expand "cmpqi_ext_3"
> (define_insn "*cmpqi_ext_3"
> (define_insn "*cmpqi_ext_4"
> (define_insn "addqi_ext_1"
> (define_insn "*addqi_ext_2"
> (define_expand "testqi_ext_1_ccno"
> (define_insn "*testqi_ext_1"
> (define_insn "*testqi_ext_2"
> (define_insn_and_split "*testqi_ext_3"
> (define_insn "andqi_ext_1"
> (define_insn "*andqi_ext_1_cc"
> (define_insn "*andqi_ext_2"
> (define_insn "*<code>qi_ext_1"
> (define_insn "*<code>qi_ext_2"
> (define_expand "xorqi_ext_1_cc"
> (define_insn "*xorqi_ext_1_cc"
>
> There are also relevant splitters and peephole2 patterns.

I see. Another approach I've looked at is removing the mode specifier from
the zero_extract in these patterns. This means that they can be of any mode
so they will match all of these modes without creating new patterns through
iterators. That also works for the testcase and passes bootstrap and testing
however there is the snag that the define_insns that don't start with a "*"
are used to generate RTL through the gen_* mechanism and in that context
the absence of a mode on the zero_extract would mean a VOIDmode zero_extract
would be created, which I'm fairly sure is not good. So in my experiments I left
those patterns alone (with an explicit SI on the zero_extract).

> IIRC, SImode zero_extract was enough to catch all high-register uses.
> There will be a pattern explosion if we want to handle all other
> integer modes here. However, I'm not a RTL expert, so someone will
> have to say what is the correct RTX form here.

Jeff, Richard, could you please give us some guidance on this issue?
Sorry for the trouble.

Thanks,
Kyrill

> Uros.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH][i386][3/3] PR target/84164: Make *cmpqi_ext_<n> patterns accept more zero_extract modes
  2018-02-09 14:50   ` Kyrill Tkachov
@ 2018-02-13 16:46     ` Jeff Law
  2018-02-14 18:04       ` Kyrill Tkachov
  0 siblings, 1 reply; 7+ messages in thread
From: Jeff Law @ 2018-02-13 16:46 UTC (permalink / raw)
  To: Kyrill Tkachov, Uros Bizjak; +Cc: gcc-patches, richard.sandiford

On 02/09/2018 07:50 AM, Kyrill Tkachov wrote:
> Hi Uros,
> 
> On 08/02/18 22:54, Uros Bizjak wrote:
>> On Thu, Feb 8, 2018 at 6:11 PM, KyrillÂ  Tkachov
>> <kyrylo.tkachov@foss.arm.com> wrote:
>>> Hi all,
>>>
>>> This patch fixes some fallout in the i386 testsuite that occurs after
>>> the
>>> simplification in patch [1/3] [1].
>>> The gcc.target/i386/extract-2.c FAILs because it expects to match:
>>> (set (reg:CC 17 flags)
>>> Â Â Â Â  (compare:CC (subreg:QI (zero_extract:SI (reg:HI 98)
>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  (const_int 8 [0x8])
>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  (const_int 8 [0x8])) 0)
>>> Â Â Â Â Â Â Â Â  (const_int 4 [0x4])))
>>>
>>> which is the *cmpqi_ext_2 pattern in i386.md but with the new
>>> simplification
>>> the combine/simplify-rtx
>>> machinery produces:
>>> (set (reg:CC 17 flags)
>>> Â Â Â Â  (compare:CC (subreg:QI (zero_extract:HI (reg:HI 98)
>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  (const_int 8 [0x8])
>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  (const_int 8 [0x8])) 0)
>>> Â Â Â Â Â Â Â Â  (const_int 4 [0x4])))
>>>
>>> Notice that the zero_extract now has HImode like the register source
>>> rather
>>> than SImode.
>>> The existing *cmpqi_ext_<n> patterns however explicitly demand an
>>> SImode on
>>> the zero_extract.
>>> I'm not overly familiar with the i386 port but I think that's too
>>> restrictive.
>>> The RTL documentation says:
>>> For (zero_extract:m loc size pos) "The mode m is the same as the mode
>>> that
>>> would be used for loc if it were a register."
>>> I'm not sure if that means that the mode of the zero_extract and the
>>> source
>>> register must always match (as is the
>>> case after patch [1/3]) but in any case it shouldn't matter semantically
>>> since we're taking a QImode subreg of the whole
>>> thing anyway.
>>>
>>> So the proposed solution in this patch is to allow HI, SI and DImode
>>> zero_extracts in these patterns as these are the
>>> modes that the ext_register_operand predicate accepts, so that the
>>> patterns
>>> can match the new form above.
>>>
>>> With this patch the aforementioned test passes again and bootstrap and
>>> testing on x86_64-unknown-linux-gnu shows
>>> no regressions.
>>>
>>> Is this ok for trunk if the first patch is accepted?
>> Huh, there are many other zero-extract patterns besides cmpqi_ext_*
>> with QImode subreg of SImode zero_extract in i386.md, used to access
>> high QImode register of HImode pair. A quick grep shows these that
>> have _ext_ in their name:
>>
>> (define_insn "*cmpqi_ext_1"
>> (define_insn "*cmpqi_ext_2"
>> (define_expand "cmpqi_ext_3"
>> (define_insn "*cmpqi_ext_3"
>> (define_insn "*cmpqi_ext_4"
>> (define_insn "addqi_ext_1"
>> (define_insn "*addqi_ext_2"
>> (define_expand "testqi_ext_1_ccno"
>> (define_insn "*testqi_ext_1"
>> (define_insn "*testqi_ext_2"
>> (define_insn_and_split "*testqi_ext_3"
>> (define_insn "andqi_ext_1"
>> (define_insn "*andqi_ext_1_cc"
>> (define_insn "*andqi_ext_2"
>> (define_insn "*<code>qi_ext_1"
>> (define_insn "*<code>qi_ext_2"
>> (define_expand "xorqi_ext_1_cc"
>> (define_insn "*xorqi_ext_1_cc"
>>
>> There are also relevant splitters and peephole2 patterns.
> 
> I see. Another approach I've looked at is removing the mode specifier from
> the zero_extract in these patterns. This means that they can be of any mode
> so they will match all of these modes without creating new patterns through
> iterators. That also works for the testcase and passes bootstrap and
> testing
> however there is the snag that the define_insns that don't start with a "*"
> are used to generate RTL through the gen_* mechanism and in that context
> the absence of a mode on the zero_extract would mean a VOIDmode
> zero_extract
> would be created, which I'm fairly sure is not good. So in my
> experiments I left
> those patterns alone (with an explicit SI on the zero_extract).
> 
>> IIRC, SImode zero_extract was enough to catch all high-register uses.
>> There will be a pattern explosion if we want to handle all other
>> integer modes here. However, I'm not a RTL expert, so someone will
>> have to say what is the correct RTX form here.
> 
> Jeff, Richard, could you please give us some guidance on this issue?
> Sorry for the trouble.
> 
I don't think any of the patterns above are known to the generic code.
So you just have to check the x86 backend to see their precise uses in a
generator (ie gen_cmpqi_ext_3) and verify those do not allow a VOIDmode
(or any other undesirable mode) to slip through.

Jeff

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH][i386][3/3] PR target/84164: Make *cmpqi_ext_<n> patterns accept more zero_extract modes
  2018-02-13 16:46     ` Jeff Law
@ 2018-02-14 18:04       ` Kyrill Tkachov
  2018-02-15  7:25         ` Uros Bizjak
  0 siblings, 1 reply; 7+ messages in thread
From: Kyrill Tkachov @ 2018-02-14 18:04 UTC (permalink / raw)
  To: Jeff Law, Uros Bizjak; +Cc: gcc-patches, richard.sandiford

[-- Attachment #1: Type: text/plain, Size: 5589 bytes --]


On 13/02/18 16:45, Jeff Law wrote:
> On 02/09/2018 07:50 AM, Kyrill Tkachov wrote:
>> Hi Uros,
>>
>> On 08/02/18 22:54, Uros Bizjak wrote:
>>> On Thu, Feb 8, 2018 at 6:11 PM, Kyrill  Tkachov
>>> <kyrylo.tkachov@foss.arm.com> wrote:
>>>> Hi all,
>>>>
>>>> This patch fixes some fallout in the i386 testsuite that occurs after
>>>> the
>>>> simplification in patch [1/3] [1].
>>>> The gcc.target/i386/extract-2.c FAILs because it expects to match:
>>>> (set (reg:CC 17 flags)
>>>>       (compare:CC (subreg:QI (zero_extract:SI (reg:HI 98)
>>>>                   (const_int 8 [0x8])
>>>>                   (const_int 8 [0x8])) 0)
>>>>           (const_int 4 [0x4])))
>>>>
>>>> which is the *cmpqi_ext_2 pattern in i386.md but with the new
>>>> simplification
>>>> the combine/simplify-rtx
>>>> machinery produces:
>>>> (set (reg:CC 17 flags)
>>>>       (compare:CC (subreg:QI (zero_extract:HI (reg:HI 98)
>>>>                   (const_int 8 [0x8])
>>>>                   (const_int 8 [0x8])) 0)
>>>>           (const_int 4 [0x4])))
>>>>
>>>> Notice that the zero_extract now has HImode like the register source
>>>> rather
>>>> than SImode.
>>>> The existing *cmpqi_ext_<n> patterns however explicitly demand an
>>>> SImode on
>>>> the zero_extract.
>>>> I'm not overly familiar with the i386 port but I think that's too
>>>> restrictive.
>>>> The RTL documentation says:
>>>> For (zero_extract:m loc size pos) "The mode m is the same as the mode
>>>> that
>>>> would be used for loc if it were a register."
>>>> I'm not sure if that means that the mode of the zero_extract and the
>>>> source
>>>> register must always match (as is the
>>>> case after patch [1/3]) but in any case it shouldn't matter semantically
>>>> since we're taking a QImode subreg of the whole
>>>> thing anyway.
>>>>
>>>> So the proposed solution in this patch is to allow HI, SI and DImode
>>>> zero_extracts in these patterns as these are the
>>>> modes that the ext_register_operand predicate accepts, so that the
>>>> patterns
>>>> can match the new form above.
>>>>
>>>> With this patch the aforementioned test passes again and bootstrap and
>>>> testing on x86_64-unknown-linux-gnu shows
>>>> no regressions.
>>>>
>>>> Is this ok for trunk if the first patch is accepted?
>>> Huh, there are many other zero-extract patterns besides cmpqi_ext_*
>>> with QImode subreg of SImode zero_extract in i386.md, used to access
>>> high QImode register of HImode pair. A quick grep shows these that
>>> have _ext_ in their name:
>>>
>>> (define_insn "*cmpqi_ext_1"
>>> (define_insn "*cmpqi_ext_2"
>>> (define_expand "cmpqi_ext_3"
>>> (define_insn "*cmpqi_ext_3"
>>> (define_insn "*cmpqi_ext_4"
>>> (define_insn "addqi_ext_1"
>>> (define_insn "*addqi_ext_2"
>>> (define_expand "testqi_ext_1_ccno"
>>> (define_insn "*testqi_ext_1"
>>> (define_insn "*testqi_ext_2"
>>> (define_insn_and_split "*testqi_ext_3"
>>> (define_insn "andqi_ext_1"
>>> (define_insn "*andqi_ext_1_cc"
>>> (define_insn "*andqi_ext_2"
>>> (define_insn "*<code>qi_ext_1"
>>> (define_insn "*<code>qi_ext_2"
>>> (define_expand "xorqi_ext_1_cc"
>>> (define_insn "*xorqi_ext_1_cc"
>>>
>>> There are also relevant splitters and peephole2 patterns.
>> I see. Another approach I've looked at is removing the mode specifier from
>> the zero_extract in these patterns. This means that they can be of any mode
>> so they will match all of these modes without creating new patterns through
>> iterators. That also works for the testcase and passes bootstrap and
>> testing
>> however there is the snag that the define_insns that don't start with a "*"
>> are used to generate RTL through the gen_* mechanism and in that context
>> the absence of a mode on the zero_extract would mean a VOIDmode
>> zero_extract
>> would be created, which I'm fairly sure is not good. So in my
>> experiments I left
>> those patterns alone (with an explicit SI on the zero_extract).
>>
>>> IIRC, SImode zero_extract was enough to catch all high-register uses.
>>> There will be a pattern explosion if we want to handle all other
>>> integer modes here. However, I'm not a RTL expert, so someone will
>>> have to say what is the correct RTX form here.
>> Jeff, Richard, could you please give us some guidance on this issue?
>> Sorry for the trouble.
>>
> I don't think any of the patterns above are known to the generic code.
> So you just have to check the x86 backend to see their precise uses in a
> generator (ie gen_cmpqi_ext_3) and verify those do not allow a VOIDmode
> (or any other undesirable mode) to slip through.
>
> Jeff

Thanks Jeff, I did have a look. I think we want to maintain the SImode on the
RTL that gets created through these named expanders, as generating a VOIDmode
zero_extract is not valid. So my patch leaves those intact.
The patch removes the mode from the zero_extract RTXes in patterns that are
only ever going to get matched (as opposed to generated). That is the ones that
start with "*" in their name.
This should allow them to match any of the zero_extract modes that
might get generated by the midend.

Bootstrapped and tested on x86_64-unknown-linux-gnu.
Is this a preferable approach?

Thanks,
Kyrill

2018-02-14  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR target/84164
     * config/i386/i386.md (*cmpqi_ext_1, *cmpqi_ext_2, *cmpqi_ext_3,
     *cmpqi_ext_4, *extzvqi_mem_rex64, *extzvqi, QImode zero_extract
     peephole, *addqi_ext_2, *testqi_ext_1, *testqi_ext_2, *andqi_ext_1_cc,
     *andqi_ext_2, *<code>qi_ext_1, *<code>qi_ext_2, *xorqi_ext_1_cc
     AND QImode subreg of zero_extract peephole): Remove mode from zero_extract.



[-- Attachment #2: i386-modes.patch --]
[-- Type: text/x-patch, Size: 6938 bytes --]

commit 7965f92e553ee915c6c8a2bd1b0c20473f732cbb
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date:   Wed Feb 7 15:46:48 2018 +0000

    [i386] Remove mode check from zero_extracts within QImode subregs

diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
index a4832bf..911a73b 100644
--- a/gcc/config/i386/i386.md
+++ b/gcc/config/i386/i386.md
@@ -1333,7 +1333,7 @@ (define_insn "*cmpqi_ext_1"
 	(compare
 	  (match_operand:QI 0 "nonimmediate_operand" "QBc,m")
 	  (subreg:QI
-	    (zero_extract:SI
+	    (zero_extract
 	      (match_operand 1 "ext_register_operand" "Q,Q")
 	      (const_int 8)
 	      (const_int 8)) 0)))]
@@ -1347,7 +1347,7 @@ (define_insn "*cmpqi_ext_2"
   [(set (reg FLAGS_REG)
 	(compare
 	  (subreg:QI
-	    (zero_extract:SI
+	    (zero_extract
 	      (match_operand 0 "ext_register_operand" "Q")
 	      (const_int 8)
 	      (const_int 8)) 0)
@@ -1372,7 +1372,7 @@ (define_insn "*cmpqi_ext_3"
   [(set (reg FLAGS_REG)
 	(compare
 	  (subreg:QI
-	    (zero_extract:SI
+	    (zero_extract
 	      (match_operand 0 "ext_register_operand" "Q,Q")
 	      (const_int 8)
 	      (const_int 8)) 0)
@@ -1387,12 +1387,12 @@ (define_insn "*cmpqi_ext_4"
   [(set (reg FLAGS_REG)
 	(compare
 	  (subreg:QI
-	    (zero_extract:SI
+	    (zero_extract
 	      (match_operand 0 "ext_register_operand" "Q")
 	      (const_int 8)
 	      (const_int 8)) 0)
 	  (subreg:QI
-	    (zero_extract:SI
+	    (zero_extract
 	      (match_operand 1 "ext_register_operand" "Q")
 	      (const_int 8)
 	      (const_int 8)) 0)))]
@@ -2931,7 +2931,7 @@ (define_expand "extzv<mode>"
 (define_insn "*extzvqi_mem_rex64"
   [(set (match_operand:QI 0 "norex_memory_operand" "=Bn")
 	(subreg:QI
-	  (zero_extract:SI (match_operand 1 "ext_register_operand" "Q")
+	  (zero_extract (match_operand 1 "ext_register_operand" "Q")
 			   (const_int 8)
 			   (const_int 8)) 0))]
   "TARGET_64BIT && reload_completed"
@@ -2952,7 +2952,7 @@ (define_insn "*extzv<mode>"
 (define_insn "*extzvqi"
   [(set (match_operand:QI 0 "nonimmediate_operand" "=QBc,?R,m")
 	(subreg:QI
-	  (zero_extract:SI (match_operand 1 "ext_register_operand" "Q,Q,Q")
+	  (zero_extract (match_operand 1 "ext_register_operand" "Q,Q,Q")
 			   (const_int 8)
 			   (const_int 8)) 0))]
   ""
@@ -2980,7 +2980,7 @@ (define_insn "*extzvqi"
 (define_peephole2
   [(set (match_operand:QI 0 "register_operand")
 	(subreg:QI
-	  (zero_extract:SI (match_operand 1 "ext_register_operand")
+	  (zero_extract (match_operand 1 "ext_register_operand")
 			   (const_int 8)
 			   (const_int 8)) 0))
    (set (match_operand:QI 2 "norex_memory_operand") (match_dup 0))]
@@ -6347,11 +6347,11 @@ (define_insn "*addqi_ext_2"
 	(subreg:SI
 	  (plus:QI
 	    (subreg:QI
-	      (zero_extract:SI (match_operand 1 "ext_register_operand" "%0")
+	      (zero_extract (match_operand 1 "ext_register_operand" "%0")
 			       (const_int 8)
 			       (const_int 8)) 0)
 	    (subreg:QI
-	      (zero_extract:SI (match_operand 2 "ext_register_operand" "Q")
+	      (zero_extract (match_operand 2 "ext_register_operand" "Q")
 			       (const_int 8)
 			       (const_int 8)) 0)) 0))
   (clobber (reg:CC FLAGS_REG))]
@@ -8601,7 +8601,7 @@ (define_insn "*testqi_ext_1"
 	(compare
 	  (and:QI
 	    (subreg:QI
-	      (zero_extract:SI (match_operand 0 "ext_register_operand" "Q,Q")
+	      (zero_extract (match_operand 0 "ext_register_operand" "Q,Q")
 			       (const_int 8)
 			       (const_int 8)) 0)
 	    (match_operand:QI 1 "general_operand" "QnBc,m"))
@@ -8617,11 +8617,11 @@ (define_insn "*testqi_ext_2"
 	(compare
 	  (and:QI
 	    (subreg:QI
-	      (zero_extract:SI (match_operand 0 "ext_register_operand" "Q")
+	      (zero_extract (match_operand 0 "ext_register_operand" "Q")
 			       (const_int 8)
 			       (const_int 8)) 0)
 	    (subreg:QI
-	      (zero_extract:SI (match_operand 1 "ext_register_operand" "Q")
+	      (zero_extract (match_operand 1 "ext_register_operand" "Q")
 			       (const_int 8)
 			       (const_int 8)) 0))
 	  (const_int 0)))]
@@ -9152,7 +9152,7 @@ (define_insn "*andqi_ext_1_cc"
 	(compare
 	  (and:QI
 	    (subreg:QI
-	      (zero_extract:SI (match_operand 1 "ext_register_operand" "0,0")
+	      (zero_extract (match_operand 1 "ext_register_operand" "0,0")
 			       (const_int 8)
 			       (const_int 8)) 0)
 	    (match_operand:QI 2 "general_operand" "QnBc,m"))
@@ -9163,7 +9163,7 @@ (define_insn "*andqi_ext_1_cc"
 	(subreg:SI
 	  (and:QI
 	    (subreg:QI
-	      (zero_extract:SI (match_dup 1)
+	      (zero_extract (match_dup 1)
 			       (const_int 8)
 			       (const_int 8)) 0)
 	    (match_dup 2)) 0))]
@@ -9182,11 +9182,11 @@ (define_insn "*andqi_ext_2"
 	(subreg:SI
 	  (and:QI
 	    (subreg:QI
-	      (zero_extract:SI (match_operand 1 "ext_register_operand" "%0")
+	      (zero_extract (match_operand 1 "ext_register_operand" "%0")
 			       (const_int 8)
 			       (const_int 8)) 0)
 	    (subreg:QI
-	      (zero_extract:SI (match_operand 2 "ext_register_operand" "Q")
+	      (zero_extract (match_operand 2 "ext_register_operand" "Q")
 			       (const_int 8)
 			       (const_int 8)) 0)) 0))
    (clobber (reg:CC FLAGS_REG))]
@@ -9571,7 +9571,7 @@ (define_insn "*<code>qi_ext_1"
 	(subreg:SI
 	  (any_or:QI
 	    (subreg:QI
-	      (zero_extract:SI (match_operand 1 "ext_register_operand" "0,0")
+	      (zero_extract (match_operand 1 "ext_register_operand" "0,0")
 			       (const_int 8)
 			       (const_int 8)) 0)
 	    (match_operand:QI 2 "general_operand" "QnBc,m")) 0))
@@ -9591,11 +9591,11 @@ (define_insn "*<code>qi_ext_2"
 	(subreg:SI
 	  (any_or:QI
 	    (subreg:QI
-	      (zero_extract:SI (match_operand 1 "ext_register_operand" "%0")
+	      (zero_extract (match_operand 1 "ext_register_operand" "%0")
 			       (const_int 8)
 			       (const_int 8)) 0)
 	    (subreg:QI
-	      (zero_extract:SI (match_operand 2 "ext_register_operand" "Q")
+	      (zero_extract (match_operand 2 "ext_register_operand" "Q")
 			       (const_int 8)
 			       (const_int 8)) 0)) 0))
    (clobber (reg:CC FLAGS_REG))]
@@ -9686,7 +9686,7 @@ (define_insn "*xorqi_ext_1_cc"
 	(compare
 	  (xor:QI
 	    (subreg:QI
-	      (zero_extract:SI (match_operand 1 "ext_register_operand" "0,0")
+	      (zero_extract (match_operand 1 "ext_register_operand" "0,0")
 			       (const_int 8)
 			       (const_int 8)) 0)
 	    (match_operand:QI 2 "general_operand" "QnBc,m"))
@@ -9697,7 +9697,7 @@ (define_insn "*xorqi_ext_1_cc"
 	(subreg:SI
 	  (xor:QI
 	    (subreg:QI
-	      (zero_extract:SI (match_dup 1)
+	      (zero_extract (match_dup 1)
 			       (const_int 8)
 			       (const_int 8)) 0)
 	  (match_dup 2)) 0))]
@@ -18800,7 +18800,7 @@ (define_peephole2
 	(match_operator 1 "compare_operator"
 	  [(and:QI
 	     (subreg:QI
-	       (zero_extract:SI (match_operand 2 "QIreg_operand")
+	       (zero_extract (match_operand 2 "QIreg_operand")
 				(const_int 8)
 				(const_int 8)) 0)
 	     (match_operand 3 "const_int_operand"))

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH][i386][3/3] PR target/84164: Make *cmpqi_ext_<n> patterns accept more zero_extract modes
  2018-02-14 18:04       ` Kyrill Tkachov
@ 2018-02-15  7:25         ` Uros Bizjak
  2018-02-16 17:38           ` Kyrill Tkachov
  0 siblings, 1 reply; 7+ messages in thread
From: Uros Bizjak @ 2018-02-15  7:25 UTC (permalink / raw)
  To: Kyrill Tkachov; +Cc: Jeff Law, gcc-patches, richard.sandiford

On Wed, Feb 14, 2018 at 7:04 PM, Kyrill  Tkachov
<kyrylo.tkachov@foss.arm.com> wrote:
>
> On 13/02/18 16:45, Jeff Law wrote:
>>
>> On 02/09/2018 07:50 AM, Kyrill Tkachov wrote:
>>>
>>> Hi Uros,
>>>
>>> On 08/02/18 22:54, Uros Bizjak wrote:
>>>>
>>>> On Thu, Feb 8, 2018 at 6:11 PM, Kyrill  Tkachov
>>>> <kyrylo.tkachov@foss.arm.com> wrote:
>>>>>
>>>>> Hi all,
>>>>>
>>>>> This patch fixes some fallout in the i386 testsuite that occurs after
>>>>> the
>>>>> simplification in patch [1/3] [1].
>>>>> The gcc.target/i386/extract-2.c FAILs because it expects to match:
>>>>> (set (reg:CC 17 flags)
>>>>>       (compare:CC (subreg:QI (zero_extract:SI (reg:HI 98)
>>>>>                   (const_int 8 [0x8])
>>>>>                   (const_int 8 [0x8])) 0)
>>>>>           (const_int 4 [0x4])))
>>>>>
>>>>> which is the *cmpqi_ext_2 pattern in i386.md but with the new
>>>>> simplification
>>>>> the combine/simplify-rtx
>>>>> machinery produces:
>>>>> (set (reg:CC 17 flags)
>>>>>       (compare:CC (subreg:QI (zero_extract:HI (reg:HI 98)
>>>>>                   (const_int 8 [0x8])
>>>>>                   (const_int 8 [0x8])) 0)
>>>>>           (const_int 4 [0x4])))
>>>>>
>>>>> Notice that the zero_extract now has HImode like the register source
>>>>> rather
>>>>> than SImode.
>>>>> The existing *cmpqi_ext_<n> patterns however explicitly demand an
>>>>> SImode on
>>>>> the zero_extract.
>>>>> I'm not overly familiar with the i386 port but I think that's too
>>>>> restrictive.
>>>>> The RTL documentation says:
>>>>> For (zero_extract:m loc size pos) "The mode m is the same as the mode
>>>>> that
>>>>> would be used for loc if it were a register."
>>>>> I'm not sure if that means that the mode of the zero_extract and the
>>>>> source
>>>>> register must always match (as is the
>>>>> case after patch [1/3]) but in any case it shouldn't matter
>>>>> semantically
>>>>> since we're taking a QImode subreg of the whole
>>>>> thing anyway.
>>>>>
>>>>> So the proposed solution in this patch is to allow HI, SI and DImode
>>>>> zero_extracts in these patterns as these are the
>>>>> modes that the ext_register_operand predicate accepts, so that the
>>>>> patterns
>>>>> can match the new form above.
>>>>>
>>>>> With this patch the aforementioned test passes again and bootstrap and
>>>>> testing on x86_64-unknown-linux-gnu shows
>>>>> no regressions.
>>>>>
>>>>> Is this ok for trunk if the first patch is accepted?
>>>>
>>>> Huh, there are many other zero-extract patterns besides cmpqi_ext_*
>>>> with QImode subreg of SImode zero_extract in i386.md, used to access
>>>> high QImode register of HImode pair. A quick grep shows these that
>>>> have _ext_ in their name:
>>>>
>>>> (define_insn "*cmpqi_ext_1"
>>>> (define_insn "*cmpqi_ext_2"
>>>> (define_expand "cmpqi_ext_3"
>>>> (define_insn "*cmpqi_ext_3"
>>>> (define_insn "*cmpqi_ext_4"
>>>> (define_insn "addqi_ext_1"
>>>> (define_insn "*addqi_ext_2"
>>>> (define_expand "testqi_ext_1_ccno"
>>>> (define_insn "*testqi_ext_1"
>>>> (define_insn "*testqi_ext_2"
>>>> (define_insn_and_split "*testqi_ext_3"
>>>> (define_insn "andqi_ext_1"
>>>> (define_insn "*andqi_ext_1_cc"
>>>> (define_insn "*andqi_ext_2"
>>>> (define_insn "*<code>qi_ext_1"
>>>> (define_insn "*<code>qi_ext_2"
>>>> (define_expand "xorqi_ext_1_cc"
>>>> (define_insn "*xorqi_ext_1_cc"
>>>>
>>>> There are also relevant splitters and peephole2 patterns.
>>>
>>> I see. Another approach I've looked at is removing the mode specifier
>>> from
>>> the zero_extract in these patterns. This means that they can be of any
>>> mode
>>> so they will match all of these modes without creating new patterns
>>> through
>>> iterators. That also works for the testcase and passes bootstrap and
>>> testing
>>> however there is the snag that the define_insns that don't start with a
>>> "*"
>>> are used to generate RTL through the gen_* mechanism and in that context
>>> the absence of a mode on the zero_extract would mean a VOIDmode
>>> zero_extract
>>> would be created, which I'm fairly sure is not good. So in my
>>> experiments I left
>>> those patterns alone (with an explicit SI on the zero_extract).
>>>
>>>> IIRC, SImode zero_extract was enough to catch all high-register uses.
>>>> There will be a pattern explosion if we want to handle all other
>>>> integer modes here. However, I'm not a RTL expert, so someone will
>>>> have to say what is the correct RTX form here.
>>>
>>> Jeff, Richard, could you please give us some guidance on this issue?
>>> Sorry for the trouble.
>>>
>> I don't think any of the patterns above are known to the generic code.
>> So you just have to check the x86 backend to see their precise uses in a
>> generator (ie gen_cmpqi_ext_3) and verify those do not allow a VOIDmode
>> (or any other undesirable mode) to slip through.
>>
>> Jeff
>
>
> Thanks Jeff, I did have a look. I think we want to maintain the SImode on
> the
> RTL that gets created through these named expanders, as generating a
> VOIDmode
> zero_extract is not valid. So my patch leaves those intact.
> The patch removes the mode from the zero_extract RTXes in patterns that are
> only ever going to get matched (as opposed to generated). That is the ones
> that
> start with "*" in their name.
> This should allow them to match any of the zero_extract modes that
> might get generated by the midend.
>
> Bootstrapped and tested on x86_64-unknown-linux-gnu.
> Is this a preferable approach?

We are trying to avoid VOIDmode RTXes as much as possible (to tighten
pattern matching to avoid various surpsrises). Looking at these
instructions, I think that using SWI248 in insn and peephole2 patterns
should be OK.

So, if it survives regression tests, the patch is OK with SWI248 mode
instead of void mode.

Thanks,
Uros.

>
> Thanks,
> Kyrill
>
> 2018-02-14  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>
>     PR target/84164
>     * config/i386/i386.md (*cmpqi_ext_1, *cmpqi_ext_2, *cmpqi_ext_3,
>     *cmpqi_ext_4, *extzvqi_mem_rex64, *extzvqi, QImode zero_extract
>     peephole, *addqi_ext_2, *testqi_ext_1, *testqi_ext_2, *andqi_ext_1_cc,
>     *andqi_ext_2, *<code>qi_ext_1, *<code>qi_ext_2, *xorqi_ext_1_cc
>     AND QImode subreg of zero_extract peephole): Remove mode from
> zero_extract.
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH][i386][3/3] PR target/84164: Make *cmpqi_ext_<n> patterns accept more zero_extract modes
  2018-02-15  7:25         ` Uros Bizjak
@ 2018-02-16 17:38           ` Kyrill Tkachov
  0 siblings, 0 replies; 7+ messages in thread
From: Kyrill Tkachov @ 2018-02-16 17:38 UTC (permalink / raw)
  To: Uros Bizjak; +Cc: Jeff Law, gcc-patches, richard.sandiford

[-- Attachment #1: Type: text/plain, Size: 8051 bytes --]


On 15/02/18 07:25, Uros Bizjak wrote:
> On Wed, Feb 14, 2018 at 7:04 PM, Kyrill  Tkachov
> <kyrylo.tkachov@foss.arm.com> wrote:
>> On 13/02/18 16:45, Jeff Law wrote:
>>> On 02/09/2018 07:50 AM, Kyrill Tkachov wrote:
>>>> Hi Uros,
>>>>
>>>> On 08/02/18 22:54, Uros Bizjak wrote:
>>>>> On Thu, Feb 8, 2018 at 6:11 PM, Kyrill  Tkachov
>>>>> <kyrylo.tkachov@foss.arm.com> wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> This patch fixes some fallout in the i386 testsuite that occurs after
>>>>>> the
>>>>>> simplification in patch [1/3] [1].
>>>>>> The gcc.target/i386/extract-2.c FAILs because it expects to match:
>>>>>> (set (reg:CC 17 flags)
>>>>>>        (compare:CC (subreg:QI (zero_extract:SI (reg:HI 98)
>>>>>>                    (const_int 8 [0x8])
>>>>>>                    (const_int 8 [0x8])) 0)
>>>>>>            (const_int 4 [0x4])))
>>>>>>
>>>>>> which is the *cmpqi_ext_2 pattern in i386.md but with the new
>>>>>> simplification
>>>>>> the combine/simplify-rtx
>>>>>> machinery produces:
>>>>>> (set (reg:CC 17 flags)
>>>>>>        (compare:CC (subreg:QI (zero_extract:HI (reg:HI 98)
>>>>>>                    (const_int 8 [0x8])
>>>>>>                    (const_int 8 [0x8])) 0)
>>>>>>            (const_int 4 [0x4])))
>>>>>>
>>>>>> Notice that the zero_extract now has HImode like the register source
>>>>>> rather
>>>>>> than SImode.
>>>>>> The existing *cmpqi_ext_<n> patterns however explicitly demand an
>>>>>> SImode on
>>>>>> the zero_extract.
>>>>>> I'm not overly familiar with the i386 port but I think that's too
>>>>>> restrictive.
>>>>>> The RTL documentation says:
>>>>>> For (zero_extract:m loc size pos) "The mode m is the same as the mode
>>>>>> that
>>>>>> would be used for loc if it were a register."
>>>>>> I'm not sure if that means that the mode of the zero_extract and the
>>>>>> source
>>>>>> register must always match (as is the
>>>>>> case after patch [1/3]) but in any case it shouldn't matter
>>>>>> semantically
>>>>>> since we're taking a QImode subreg of the whole
>>>>>> thing anyway.
>>>>>>
>>>>>> So the proposed solution in this patch is to allow HI, SI and DImode
>>>>>> zero_extracts in these patterns as these are the
>>>>>> modes that the ext_register_operand predicate accepts, so that the
>>>>>> patterns
>>>>>> can match the new form above.
>>>>>>
>>>>>> With this patch the aforementioned test passes again and bootstrap and
>>>>>> testing on x86_64-unknown-linux-gnu shows
>>>>>> no regressions.
>>>>>>
>>>>>> Is this ok for trunk if the first patch is accepted?
>>>>> Huh, there are many other zero-extract patterns besides cmpqi_ext_*
>>>>> with QImode subreg of SImode zero_extract in i386.md, used to access
>>>>> high QImode register of HImode pair. A quick grep shows these that
>>>>> have _ext_ in their name:
>>>>>
>>>>> (define_insn "*cmpqi_ext_1"
>>>>> (define_insn "*cmpqi_ext_2"
>>>>> (define_expand "cmpqi_ext_3"
>>>>> (define_insn "*cmpqi_ext_3"
>>>>> (define_insn "*cmpqi_ext_4"
>>>>> (define_insn "addqi_ext_1"
>>>>> (define_insn "*addqi_ext_2"
>>>>> (define_expand "testqi_ext_1_ccno"
>>>>> (define_insn "*testqi_ext_1"
>>>>> (define_insn "*testqi_ext_2"
>>>>> (define_insn_and_split "*testqi_ext_3"
>>>>> (define_insn "andqi_ext_1"
>>>>> (define_insn "*andqi_ext_1_cc"
>>>>> (define_insn "*andqi_ext_2"
>>>>> (define_insn "*<code>qi_ext_1"
>>>>> (define_insn "*<code>qi_ext_2"
>>>>> (define_expand "xorqi_ext_1_cc"
>>>>> (define_insn "*xorqi_ext_1_cc"
>>>>>
>>>>> There are also relevant splitters and peephole2 patterns.
>>>> I see. Another approach I've looked at is removing the mode specifier
>>>> from
>>>> the zero_extract in these patterns. This means that they can be of any
>>>> mode
>>>> so they will match all of these modes without creating new patterns
>>>> through
>>>> iterators. That also works for the testcase and passes bootstrap and
>>>> testing
>>>> however there is the snag that the define_insns that don't start with a
>>>> "*"
>>>> are used to generate RTL through the gen_* mechanism and in that context
>>>> the absence of a mode on the zero_extract would mean a VOIDmode
>>>> zero_extract
>>>> would be created, which I'm fairly sure is not good. So in my
>>>> experiments I left
>>>> those patterns alone (with an explicit SI on the zero_extract).
>>>>
>>>>> IIRC, SImode zero_extract was enough to catch all high-register uses.
>>>>> There will be a pattern explosion if we want to handle all other
>>>>> integer modes here. However, I'm not a RTL expert, so someone will
>>>>> have to say what is the correct RTX form here.
>>>> Jeff, Richard, could you please give us some guidance on this issue?
>>>> Sorry for the trouble.
>>>>
>>> I don't think any of the patterns above are known to the generic code.
>>> So you just have to check the x86 backend to see their precise uses in a
>>> generator (ie gen_cmpqi_ext_3) and verify those do not allow a VOIDmode
>>> (or any other undesirable mode) to slip through.
>>>
>>> Jeff
>>
>> Thanks Jeff, I did have a look. I think we want to maintain the SImode on
>> the
>> RTL that gets created through these named expanders, as generating a
>> VOIDmode
>> zero_extract is not valid. So my patch leaves those intact.
>> The patch removes the mode from the zero_extract RTXes in patterns that are
>> only ever going to get matched (as opposed to generated). That is the ones
>> that
>> start with "*" in their name.
>> This should allow them to match any of the zero_extract modes that
>> might get generated by the midend.
>>
>> Bootstrapped and tested on x86_64-unknown-linux-gnu.
>> Is this a preferable approach?
> We are trying to avoid VOIDmode RTXes as much as possible (to tighten
> pattern matching to avoid various surpsrises). Looking at these
> instructions, I think that using SWI248 in insn and peephole2 patterns
> should be OK.
>
> So, if it survives regression tests, the patch is OK with SWI248 mode
> instead of void mode.
>
> Thanks,
> Uros.


Thanks Uros, here's my proposed patch.
If we're adding the mode iterator we also have to adjust the name of the patterns I believe,
so this patch does that too.

Bootstrapped and tested on x86_64-unknown-linux-gnu.
I will commit it if the prerequisite patch at https://gcc.gnu.org/ml/gcc-patches/2018-02/msg00443.html
is approved.

Thank you for your guidance,
Kyrill

2018-02-16  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR target/84164
     * config/i386/i386.md (*cmpqi_ext_1): Rename to...
     (*cmpqi<mode>_ext_1): ... This.  Use SWI248 for zero_extract mode.
     (*cmpqi<mode>_ext_2): Rename to...
     (*cmpqi_ext_2): ... This.  Use SWI248 for zero_extract mode.
     (*cmpqi_ext_3): Rename to...
     (*cmpqi<mode>_ext_3): ... This.  Use SWI248 for zero_extract mode.
     (*cmpqi_ext_4): Rename to...
     (*cmpqi<mode>_ext_4): ... This.  Use SWI248 for zero_extract mode.
     (*extzvqi_mem_rex64): Rename to...
     (*extzvqi<mode>_mem_rex64): ... This.  Use SWI248 for zero_extract
     mode.
     (*extzvqi): Rename to..
     (*extzvqi<mode>): ... This.  Use SWI248 for zero_extract mode.
     (QImode zero_extract peephole): Use SWI248 for zero_extract mode.
     (*addqi_ext_2): Rename to...
     (*addqi<mode>_ext_2): ... This.  Use SWI248 for zero_extract mode.
     (*testqi_ext_1): Rename to...
     (*testqi<mode>_ext_1): ... This.  Use SWI248 for zero_extract mode.
     (*testqi_ext_2): Rename to...
     (*testqi<mode>_ext_2): ... This.  Use SWI248 for zero_extract mode.
     (*andqi_ext_1_cc): Rename to...
     (*andqi<mode>_ext_1_cc): ... This.  Use SWI248 for zero_extract mode.
     (*andqi_ext_2): Rename to...
     (*andqi<mode>_ext_2): ... This.  Use SWI248 for zero_extract mode.
     (*<code>qi_ext_1): Rename to...
     (*<code>qi<mode>_ext_1): ... This.  Use SWI248 for zero_extract mode.
     (*<code>qi_ext_2): Rename to...
     (*<code>qi<mode>_ext_2): ... This.  Use SWI248 for zero_extract mode.
     (*xorqi_ext_1_cc): Rename to...
     (*xorqi<mode>_ext_1_cc): ... This.  Use SWI248 for zero_extract mode.
     (AND QImode subreg of zero_extract peephole): Use SWI248 for
     zero_extract mode.

[-- Attachment #2: i386-modes.patch --]
[-- Type: text/x-patch, Size: 9932 bytes --]

commit 864d03bba8274e1281142e389961b499d6cbf81b
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date:   Wed Feb 7 15:46:48 2018 +0000

    [i386] Make QImode subregs of zero_extracts more robust

diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
index 3998053..c5a7d88 100644
--- a/gcc/config/i386/i386.md
+++ b/gcc/config/i386/i386.md
@@ -1328,12 +1328,12 @@ (define_insn "*cmp<mode>_minus_1"
   [(set_attr "type" "icmp")
    (set_attr "mode" "<MODE>")])
 
-(define_insn "*cmpqi_ext_1"
+(define_insn "*cmpqi<mode>_ext_1"
   [(set (reg FLAGS_REG)
 	(compare
 	  (match_operand:QI 0 "nonimmediate_operand" "QBc,m")
 	  (subreg:QI
-	    (zero_extract:SI
+	    (zero_extract:SWI248
 	      (match_operand 1 "ext_register_operand" "Q,Q")
 	      (const_int 8)
 	      (const_int 8)) 0)))]
@@ -1343,11 +1343,11 @@ (define_insn "*cmpqi_ext_1"
    (set_attr "type" "icmp")
    (set_attr "mode" "QI")])
 
-(define_insn "*cmpqi_ext_2"
+(define_insn "*cmpqi<mode>_ext_2"
   [(set (reg FLAGS_REG)
 	(compare
 	  (subreg:QI
-	    (zero_extract:SI
+	    (zero_extract:SWI248
 	      (match_operand 0 "ext_register_operand" "Q")
 	      (const_int 8)
 	      (const_int 8)) 0)
@@ -1368,11 +1368,11 @@ (define_expand "cmpqi_ext_3"
 	      (const_int 8)) 0)
 	  (match_operand:QI 1 "const_int_operand")))])
 
-(define_insn "*cmpqi_ext_3"
+(define_insn "*cmpqi<mode>_ext_3"
   [(set (reg FLAGS_REG)
 	(compare
 	  (subreg:QI
-	    (zero_extract:SI
+	    (zero_extract:SWI248
 	      (match_operand 0 "ext_register_operand" "Q,Q")
 	      (const_int 8)
 	      (const_int 8)) 0)
@@ -1383,16 +1383,16 @@ (define_insn "*cmpqi_ext_3"
    (set_attr "type" "icmp")
    (set_attr "mode" "QI")])
 
-(define_insn "*cmpqi_ext_4"
+(define_insn "*cmpqi<mode>_ext_4"
   [(set (reg FLAGS_REG)
 	(compare
 	  (subreg:QI
-	    (zero_extract:SI
+	    (zero_extract:SWI248
 	      (match_operand 0 "ext_register_operand" "Q")
 	      (const_int 8)
 	      (const_int 8)) 0)
 	  (subreg:QI
-	    (zero_extract:SI
+	    (zero_extract:SWI248
 	      (match_operand 1 "ext_register_operand" "Q")
 	      (const_int 8)
 	      (const_int 8)) 0)))]
@@ -2928,10 +2928,10 @@ (define_expand "extzv<mode>"
     operands[1] = copy_to_reg (operands[1]);
 })
 
-(define_insn "*extzvqi_mem_rex64"
+(define_insn "*extzvqi<mode>_mem_rex64"
   [(set (match_operand:QI 0 "norex_memory_operand" "=Bn")
 	(subreg:QI
-	  (zero_extract:SI (match_operand 1 "ext_register_operand" "Q")
+	  (zero_extract:SWI248 (match_operand 1 "ext_register_operand" "Q")
 			   (const_int 8)
 			   (const_int 8)) 0))]
   "TARGET_64BIT && reload_completed"
@@ -2949,10 +2949,10 @@ (define_insn "*extzv<mode>"
   [(set_attr "type" "imovx")
    (set_attr "mode" "SI")])
 
-(define_insn "*extzvqi"
+(define_insn "*extzvqi<mode>"
   [(set (match_operand:QI 0 "nonimmediate_operand" "=QBc,?R,m")
 	(subreg:QI
-	  (zero_extract:SI (match_operand 1 "ext_register_operand" "Q,Q,Q")
+	  (zero_extract:SWI248 (match_operand 1 "ext_register_operand" "Q,Q,Q")
 			   (const_int 8)
 			   (const_int 8)) 0))]
   ""
@@ -2980,7 +2980,7 @@ (define_insn "*extzvqi"
 (define_peephole2
   [(set (match_operand:QI 0 "register_operand")
 	(subreg:QI
-	  (zero_extract:SI (match_operand 1 "ext_register_operand")
+	  (zero_extract:SWI248 (match_operand 1 "ext_register_operand")
 			   (const_int 8)
 			   (const_int 8)) 0))
    (set (match_operand:QI 2 "norex_memory_operand") (match_dup 0))]
@@ -6340,18 +6340,18 @@ (define_insn "addqi_ext_1"
 	(const_string "alu")))
    (set_attr "mode" "QI")])
 
-(define_insn "*addqi_ext_2"
+(define_insn "*addqi<mode>_ext_2"
   [(set (zero_extract:SI (match_operand 0 "ext_register_operand" "+Q")
 			 (const_int 8)
 			 (const_int 8))
 	(subreg:SI
 	  (plus:QI
 	    (subreg:QI
-	      (zero_extract:SI (match_operand 1 "ext_register_operand" "%0")
+	      (zero_extract:SWI248 (match_operand 1 "ext_register_operand" "%0")
 			       (const_int 8)
 			       (const_int 8)) 0)
 	    (subreg:QI
-	      (zero_extract:SI (match_operand 2 "ext_register_operand" "Q")
+	      (zero_extract:SWI248 (match_operand 2 "ext_register_operand" "Q")
 			       (const_int 8)
 			       (const_int 8)) 0)) 0))
   (clobber (reg:CC FLAGS_REG))]
@@ -8596,12 +8596,12 @@ (define_expand "testqi_ext_1_ccno"
 	      (match_operand 1 "const_int_operand"))
 	  (const_int 0)))])
 
-(define_insn "*testqi_ext_1"
+(define_insn "*testqi<mode>_ext_1"
   [(set (reg FLAGS_REG)
 	(compare
 	  (and:QI
 	    (subreg:QI
-	      (zero_extract:SI (match_operand 0 "ext_register_operand" "Q,Q")
+	      (zero_extract:SWI248 (match_operand 0 "ext_register_operand" "Q,Q")
 			       (const_int 8)
 			       (const_int 8)) 0)
 	    (match_operand:QI 1 "general_operand" "QnBc,m"))
@@ -8612,16 +8612,16 @@ (define_insn "*testqi_ext_1"
    (set_attr "type" "test")
    (set_attr "mode" "QI")])
 
-(define_insn "*testqi_ext_2"
+(define_insn "*testqi<mode>_ext_2"
   [(set (reg FLAGS_REG)
 	(compare
 	  (and:QI
 	    (subreg:QI
-	      (zero_extract:SI (match_operand 0 "ext_register_operand" "Q")
+	      (zero_extract:SWI248 (match_operand 0 "ext_register_operand" "Q")
 			       (const_int 8)
 			       (const_int 8)) 0)
 	    (subreg:QI
-	      (zero_extract:SI (match_operand 1 "ext_register_operand" "Q")
+	      (zero_extract:SWI248 (match_operand 1 "ext_register_operand" "Q")
 			       (const_int 8)
 			       (const_int 8)) 0))
 	  (const_int 0)))]
@@ -9147,12 +9147,12 @@ (define_insn "andqi_ext_1"
 
 ;; Generated by peephole translating test to and.  This shows up
 ;; often in fp comparisons.
-(define_insn "*andqi_ext_1_cc"
+(define_insn "*andqi<mode>_ext_1_cc"
   [(set (reg FLAGS_REG)
 	(compare
 	  (and:QI
 	    (subreg:QI
-	      (zero_extract:SI (match_operand 1 "ext_register_operand" "0,0")
+	      (zero_extract:SWI248 (match_operand 1 "ext_register_operand" "0,0")
 			       (const_int 8)
 			       (const_int 8)) 0)
 	    (match_operand:QI 2 "general_operand" "QnBc,m"))
@@ -9163,7 +9163,7 @@ (define_insn "*andqi_ext_1_cc"
 	(subreg:SI
 	  (and:QI
 	    (subreg:QI
-	      (zero_extract:SI (match_dup 1)
+	      (zero_extract:SWI248 (match_dup 1)
 			       (const_int 8)
 			       (const_int 8)) 0)
 	    (match_dup 2)) 0))]
@@ -9175,18 +9175,18 @@ (define_insn "*andqi_ext_1_cc"
    (set_attr "type" "alu")
    (set_attr "mode" "QI")])
 
-(define_insn "*andqi_ext_2"
+(define_insn "*andqi<mode>_ext_2"
   [(set (zero_extract:SI (match_operand 0 "ext_register_operand" "+Q")
 			 (const_int 8)
 			 (const_int 8))
 	(subreg:SI
 	  (and:QI
 	    (subreg:QI
-	      (zero_extract:SI (match_operand 1 "ext_register_operand" "%0")
+	      (zero_extract:SWI248 (match_operand 1 "ext_register_operand" "%0")
 			       (const_int 8)
 			       (const_int 8)) 0)
 	    (subreg:QI
-	      (zero_extract:SI (match_operand 2 "ext_register_operand" "Q")
+	      (zero_extract:SWI248 (match_operand 2 "ext_register_operand" "Q")
 			       (const_int 8)
 			       (const_int 8)) 0)) 0))
    (clobber (reg:CC FLAGS_REG))]
@@ -9564,14 +9564,14 @@ (define_insn "*<code><mode>_3"
   [(set_attr "type" "alu")
    (set_attr "mode" "<MODE>")])
 
-(define_insn "*<code>qi_ext_1"
+(define_insn "*<code>qi<mode>_ext_1"
   [(set (zero_extract:SI (match_operand 0 "ext_register_operand" "+Q,Q")
 			 (const_int 8)
 			 (const_int 8))
 	(subreg:SI
 	  (any_or:QI
 	    (subreg:QI
-	      (zero_extract:SI (match_operand 1 "ext_register_operand" "0,0")
+	      (zero_extract:SWI248 (match_operand 1 "ext_register_operand" "0,0")
 			       (const_int 8)
 			       (const_int 8)) 0)
 	    (match_operand:QI 2 "general_operand" "QnBc,m")) 0))
@@ -9584,18 +9584,18 @@ (define_insn "*<code>qi_ext_1"
    (set_attr "type" "alu")
    (set_attr "mode" "QI")])
 
-(define_insn "*<code>qi_ext_2"
+(define_insn "*<code>qi<mode>_ext_2"
   [(set (zero_extract:SI (match_operand 0 "ext_register_operand" "+Q")
 			 (const_int 8)
 			 (const_int 8))
 	(subreg:SI
 	  (any_or:QI
 	    (subreg:QI
-	      (zero_extract:SI (match_operand 1 "ext_register_operand" "%0")
+	      (zero_extract:SWI248 (match_operand 1 "ext_register_operand" "%0")
 			       (const_int 8)
 			       (const_int 8)) 0)
 	    (subreg:QI
-	      (zero_extract:SI (match_operand 2 "ext_register_operand" "Q")
+	      (zero_extract:SWI248 (match_operand 2 "ext_register_operand" "Q")
 			       (const_int 8)
 			       (const_int 8)) 0)) 0))
    (clobber (reg:CC FLAGS_REG))]
@@ -9681,12 +9681,12 @@ (define_expand "xorqi_ext_1_cc"
 				 (const_int 8)) 0)
 	    (match_dup 2)) 0))])])
 
-(define_insn "*xorqi_ext_1_cc"
+(define_insn "*xorqi<mode>_ext_1_cc"
   [(set (reg FLAGS_REG)
 	(compare
 	  (xor:QI
 	    (subreg:QI
-	      (zero_extract:SI (match_operand 1 "ext_register_operand" "0,0")
+	      (zero_extract:SWI248 (match_operand 1 "ext_register_operand" "0,0")
 			       (const_int 8)
 			       (const_int 8)) 0)
 	    (match_operand:QI 2 "general_operand" "QnBc,m"))
@@ -9697,7 +9697,7 @@ (define_insn "*xorqi_ext_1_cc"
 	(subreg:SI
 	  (xor:QI
 	    (subreg:QI
-	      (zero_extract:SI (match_dup 1)
+	      (zero_extract:SWI248 (match_dup 1)
 			       (const_int 8)
 			       (const_int 8)) 0)
 	  (match_dup 2)) 0))]
@@ -18800,7 +18800,7 @@ (define_peephole2
 	(match_operator 1 "compare_operator"
 	  [(and:QI
 	     (subreg:QI
-	       (zero_extract:SI (match_operand 2 "QIreg_operand")
+	       (zero_extract:SWI248 (match_operand 2 "QIreg_operand")
 				(const_int 8)
 				(const_int 8)) 0)
 	     (match_operand 3 "const_int_operand"))
@@ -18814,7 +18814,7 @@ (define_peephole2
 	   (match_op_dup 1
 	     [(and:QI
 		(subreg:QI
-		  (zero_extract:SI (match_dup 2)
+		  (zero_extract:SWI248 (match_dup 2)
 				   (const_int 8)
 				   (const_int 8)) 0)
 		(match_dup 3))
@@ -18825,7 +18825,7 @@ (define_peephole2
 	   (subreg:SI
 	     (and:QI
 	       (subreg:QI
-		 (zero_extract:SI (match_dup 2)
+		 (zero_extract:SWI248 (match_dup 2)
 				  (const_int 8)
 				  (const_int 8)) 0)
 	       (match_dup 3)) 0))])])

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2018-02-16 17:38 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-08 17:11 [PATCH][i386][3/3] PR target/84164: Make *cmpqi_ext_<n> patterns accept more zero_extract modes Kyrill Tkachov
2018-02-08 22:54 ` Uros Bizjak
2018-02-09 14:50   ` Kyrill Tkachov
2018-02-13 16:46     ` Jeff Law
2018-02-14 18:04       ` Kyrill Tkachov
2018-02-15  7:25         ` Uros Bizjak
2018-02-16 17:38           ` Kyrill Tkachov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).