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

[Jeff Law]

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