Hi Uros,
On 08/02/18 22:54, Uros Bizjak wrote:
On Thu, Feb 8, 2018 at 6:11 PM, Kyrill Tkachov
<kyrylo.tkac...@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.
