Re: [RFA][PATCH][PR tree-optimization/64910] x86 backend improvement

Re: [RFA][PATCH][PR tree-optimization/64910] x86 backend improvement

[Uros Bizjak]

Hello!

+ 2015-12-19  Jeff Law  <law@redhat.com>
+
+ PR tree-optimization/64910
+ * config/i386/i386.md (testqi_ext_3): Allow HImode.

OK for mainline and branch.

Thanks,
Uros.

Re: [RFA][PATCH][PR tree-optimization/64910] x86 backend improvement

[Jeff Law]

On 12/19/2015 11:06 AM, Uros Bizjak wrote:
> Hello!
>
> + 2015-12-19  Jeff Law  <law@redhat.com>
> +
> + PR tree-optimization/64910
> + * config/i386/i386.md (testqi_ext_3): Allow HImode.
>
> OK for mainline and branch.
Thanks.  I double-checked and gcc-5 has not regressed, presumably 
there's an additional interaction that's causing this to only manifest 
on the trunk.  Perhaps in combine or in the optabs bits that figure out 
the modes to use on these insns.

Given gcc-5 isn't regressing, I didn't install the patch on the branch.

Jeff

[RFA][PATCH][PR tree-optimization/64910] x86 backend improvement

[Jeff Law]

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

This patch addresses a deficiency found when testing a patch to fix 
undesirable gimple code created by reassociation (pr64910).  Based on 
what I've seen from a code generation standpoint before/after this 
patch, I would say it's highly likely this is fixing a regression in gcc-5.

[ This is a piece of 64910 -- essentially it avoids regressing code 
generation when the fix for 64910 is applied. ]

combine.c::make_extraction will query the backend for the correct modes 
to use for field extractions.  Essentially it looks at the backend's 
extraction pattern. In 4.9 for x86 we had:

(define_expand "extzv"
   [(set (match_operand:SI 0 "register_operand")
         (zero_extract:SI (match_operand 1 "ext_register_operand")
                          (match_operand:SI 2 "const8_operand")
                          (match_operand:SI 3 "const8_operand")))]


make_extraction looks at the modes of the various parts of the 
zero_extract rtx and uses it to create a suitable zero_extract 
expression & operands.  If the rtx/operand has no mode, word_mode will 
be used.  Note carefully the mode of the zero-extract -- SImode in this 
case.

Anyway, the result of make_extraction is embedded into other patterns 
like this:


(set (flags) (compare (zero_extract:MODE ...) (const_int 0))

Which will be then passed to recog to try and match a backend pattern.

Again in 4.9 we have:

(define_insn "*testqi_ext_3"
   [(set (reg FLAGS_REG)
         (compare (zero_extract:SWI48
                    (match_operand 0 "nonimmediate_operand" "rm")
                    (match_operand:SWI48 1 "const_int_operand")
                    (match_operand:SWI48 2 "const_int_operand"))
                  (const_int 0)))]



So note how the testqi_ext_3 pattern's modes (SWI48, ie, SI/DI) are a 
strict superset of those in the extzv expander for the zero_extract rtx 
(SI).

That's all fine and good. However, if we look at the trunk (or gcc-5) we 
have:

(define_expand "extzv<mode>"
   [(set (match_operand:SWI248 0 "register_operand")
         (zero_extract:SWI248 (match_operand:SWI248 1 "register_operand")
                              (match_operand:SI 2 "const_int_operand")
                              (match_operand:SI 3 "const_int_operand")))]

And

(define_insn "*testqi_ext_3"
   [(set (reg FLAGS_REG)
         (compare (zero_extract:SWI48
                    (match_operand 0 "nonimmediate_operand" "rm")
                    (match_operand 1 "const_int_operand" "n")
                    (match_operand 2 "const_int_operand" "n"))
                  (const_int 0)))]



Note how the mode of the zero_extract in the expander allows HImode 
(SWI248), but the mode of the zero_extract in testqi_ext_3 does not 
allow HImode (SWI48).


So make_extraction will create

(set (flags) (compare (zero_extract:HI (...) (const_int 0))

But testqi_ext_3 does not support HImode on the zero_extract and the 
pattern doesn't match which cripples combine's ability to create these 
insns.

Fixing this results in consistently better code by eliminating setup 
code for bit tests.  It's quite pervasive in GCC itself.

This has been bootstrapped and regression tested on x86_64-linux-gnu, 
with and without the rest of the fix for 64910.  The test has been 
tested on x86 and x86-64.

OK for the trunk?



[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 2087 bytes --]

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 1caa076..e6ec3cd 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,8 @@
+2015-12-19  Jeff Law  <law@redhat.com>
+
+	PR tree-optimization/64910
+	* config/i386/i386.md (testqi_ext_3): Allow HImode.
+
 2015-12-18  Jeff Law  <law@redhat.com>
 
 	PR rtl-optimization/49847
diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
index 49b2216..e8178f3 100644
--- a/gcc/config/i386/i386.md
+++ b/gcc/config/i386/i386.md
@@ -7868,7 +7868,7 @@
 ;; Combine likes to form bit extractions for some tests.  Humor it.
 (define_insn "*testqi_ext_3"
   [(set (reg FLAGS_REG)
-	(compare (zero_extract:SWI48
+	(compare (zero_extract:SWI248
 		   (match_operand 0 "nonimmediate_operand" "rm")
 		   (match_operand 1 "const_int_operand" "n")
 		   (match_operand 2 "const_int_operand" "n"))
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 57326d1..143c73c 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2015-12-19  Jeff Law  <law@redhat.com>
+
+	PR tree-optimization/64910
+	* gcc.target/i386/bittest.c: New test.
+
 2015-12-18  Jeff Law  <law@redhat.com>
 
 	PR rtl-optimization/49847
diff --git a/gcc/testsuite/gcc.target/i386/bittest.c b/gcc/testsuite/gcc.target/i386/bittest.c
new file mode 100644
index 0000000..7b7ce9e
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/bittest.c
@@ -0,0 +1,22 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+extern int dbg_cnt (void);
+
+struct function
+{
+  unsigned int calls_setjmp:1;
+};
+extern struct function *cfun;
+unsigned char
+gate_rtl_cprop (void)
+{
+  return !(cfun + 0)->calls_setjmp && dbg_cnt ();
+}
+
+/* This should be implementable without performing a bitmask as we can
+   just use a test imm,mem.  So instructions which load the object from
+   memory and mask off bits are unnecessary.  In theory we can just count
+   the move-with-extension, and and testb instructions.  There should be
+   only one.  */
+/* { dg-final { scan-assembler-times "movzbl|and|testb" 1 { target { i?86-*-* x86_64-*-*} } } } */