From: Kyrill Tkachov <kyrylo.tkachov@foss.arm.com>
To: Uros Bizjak <ubizjak@gmail.com>
Cc: Jeff Law <law@redhat.com>,
"gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>,
richard.sandiford@linaro.org
Subject: Re: [PATCH][i386][3/3] PR target/84164: Make *cmpqi_ext_<n> patterns accept more zero_extract modes
Date: Fri, 16 Feb 2018 17:38:00 -0000 [thread overview]
Message-ID: <5A8716FA.6090208@foss.arm.com> (raw)
In-Reply-To: <CAFULd4ZU+Pa5dPT_CRFmooRZFaHtnk8_rk3hrxBWvV0+x9VSiQ@mail.gmail.com>
[-- 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))])])
prev parent reply other threads:[~2018-02-16 17:38 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-02-08 17:11 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 message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5A8716FA.6090208@foss.arm.com \
--to=kyrylo.tkachov@foss.arm.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=law@redhat.com \
--cc=richard.sandiford@linaro.org \
--cc=ubizjak@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).