Message ID | 56A11A98.1020503@foss.arm.com |
---|---|
State | New |
Headers | show |
Hi Kyrill, the patched gcc generates correct asm for me for the test case. (I'll then build the whole system to see if other it-block related bugs are gone too.) One short question, the newly generated RTL for x = x | <cond> (a) will be orr %0, %1, #1; it <cond>; mov%D2\\t%0, %1 (b) The cond in (a) should be the reverse of cond in(b), right? Thanks for your quick fix. Han On Thu, Jan 21, 2016 at 9:51 AM, Kyrill Tkachov <kyrylo.tkachov@foss.arm.com> wrote: > Hi all, > > In this wrong-code PR the pattern for performing > x = x | <cond> for -mrestrict-it is buggy and ends up writing 1 to the > result register rather than orring it. > > The offending pattern is *thumb2_ior_scc_strict_it. > My proposed solution is to rewrite it as a splitter, remove the > alternative for the case where operands[1] and 0 are the same reg > that emits the bogus: > it <cond>; mov<cond>%0, #1; it <cond>; orr<cond> %0, %1 > > to emit the RTL equivalent to: > orr %0, %1, #1; it <cond>; mov%D2\\t%0, %1 > while marking operand 0 as an earlyclobber operand so that it doesn't > get assigned the same register as operand 1. > > This way we avoid the wrong-code, make the sequence better (by eliminating > the move of #1 into a register > and relaxing the constraints from 'l' to 'r' since only the register move > has to be conditional). > and still stay within the rules for arm_restrict_it. > > Bootstrapped and tested on arm-none-linux-gnueabihf configured > --with-arch=armv8-a and --with-mode=thumb. > > Ok for trunk, GCC 5 and 4.9? > > Han, can you please try this out to see if it solves the problem on your end > as well? > > Thanks, > Kyrill > > 2016-01-21 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > PR target/69403 > * config/arm/thumb2.md (*thumb2_ior_scc_strict_it): Convert to > define_insn_and_split. Ensure operands[1] and operands[0] do not > get assigned the same register. > > 2016-01-21 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > PR target/69403 > * gcc.c-torture/execute/pr69403.c: New test.
Hi Han, On 21/01/16 22:57, Han Shen wrote: > Hi Kyrill, the patched gcc generates correct asm for me for the test > case. (I'll then build the whole system to see if other it-block > related bugs are gone too.) > > One short question, the newly generated RTL for > x = x | <cond> (a) > will be > orr %0, %1, #1; it <cond>; mov%D2\\t%0, %1 (b) > > The cond in (a) should be the reverse of cond in(b), right? yes, the first C-like expression is just some pseudocode. I guess it would be better written as: x = x | <comparison result>. Thanks for trying it out. I bootstrapped and tested this patch on trunk and GCC 5. I'll be doing the same on the 4.9 branch. Kyrill > Thanks for your quick fix. > > Han > > On Thu, Jan 21, 2016 at 9:51 AM, Kyrill Tkachov > <kyrylo.tkachov@foss.arm.com> wrote: >> Hi all, >> >> In this wrong-code PR the pattern for performing >> x = x | <cond> for -mrestrict-it is buggy and ends up writing 1 to the >> result register rather than orring it. >> >> The offending pattern is *thumb2_ior_scc_strict_it. >> My proposed solution is to rewrite it as a splitter, remove the >> alternative for the case where operands[1] and 0 are the same reg >> that emits the bogus: >> it <cond>; mov<cond>%0, #1; it <cond>; orr<cond> %0, %1 >> >> to emit the RTL equivalent to: >> orr %0, %1, #1; it <cond>; mov%D2\\t%0, %1 >> while marking operand 0 as an earlyclobber operand so that it doesn't >> get assigned the same register as operand 1. >> >> This way we avoid the wrong-code, make the sequence better (by eliminating >> the move of #1 into a register >> and relaxing the constraints from 'l' to 'r' since only the register move >> has to be conditional). >> and still stay within the rules for arm_restrict_it. >> >> Bootstrapped and tested on arm-none-linux-gnueabihf configured >> --with-arch=armv8-a and --with-mode=thumb. >> >> Ok for trunk, GCC 5 and 4.9? >> >> Han, can you please try this out to see if it solves the problem on your end >> as well? >> >> Thanks, >> Kyrill >> >> 2016-01-21 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >> >> PR target/69403 >> * config/arm/thumb2.md (*thumb2_ior_scc_strict_it): Convert to >> define_insn_and_split. Ensure operands[1] and operands[0] do not >> get assigned the same register. >> >> 2016-01-21 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >> >> PR target/69403 >> * gcc.c-torture/execute/pr69403.c: New test. > >
On Fri, Jan 22, 2016 at 9:32 AM, Kyrill Tkachov <kyrylo.tkachov@foss.arm.com> wrote: > Hi Han, > > On 21/01/16 22:57, Han Shen wrote: >> >> Hi Kyrill, the patched gcc generates correct asm for me for the test >> case. (I'll then build the whole system to see if other it-block >> related bugs are gone too.) >> >> One short question, the newly generated RTL for >> x = x | <cond> (a) >> will be >> orr %0, %1, #1; it <cond>; mov%D2\\t%0, %1 (b) >> >> The cond in (a) should be the reverse of cond in(b), right? > > > yes, the first C-like expression is just some pseudocode. > I guess it would be better written as: > x = x | <comparison result>. > > Thanks for trying it out. > I bootstrapped and tested this patch on trunk and GCC 5. > I'll be doing the same on the 4.9 branch. Ok everywhere. While you are here could you also audit the other patterns that we changed for restrict_it to check that this thinko isn't present anywhere else, please ? Ramana > > Kyrill > > >> Thanks for your quick fix. >> >> Han >> >> On Thu, Jan 21, 2016 at 9:51 AM, Kyrill Tkachov >> <kyrylo.tkachov@foss.arm.com> wrote: >>> >>> Hi all, >>> >>> In this wrong-code PR the pattern for performing >>> x = x | <cond> for -mrestrict-it is buggy and ends up writing 1 to the >>> result register rather than orring it. >>> >>> The offending pattern is *thumb2_ior_scc_strict_it. >>> My proposed solution is to rewrite it as a splitter, remove the >>> alternative for the case where operands[1] and 0 are the same reg >>> that emits the bogus: >>> it <cond>; mov<cond>%0, #1; it <cond>; orr<cond> %0, %1 >>> >>> to emit the RTL equivalent to: >>> orr %0, %1, #1; it <cond>; mov%D2\\t%0, %1 >>> while marking operand 0 as an earlyclobber operand so that it doesn't >>> get assigned the same register as operand 1. >>> >>> This way we avoid the wrong-code, make the sequence better (by >>> eliminating >>> the move of #1 into a register >>> and relaxing the constraints from 'l' to 'r' since only the register move >>> has to be conditional). >>> and still stay within the rules for arm_restrict_it. >>> >>> Bootstrapped and tested on arm-none-linux-gnueabihf configured >>> --with-arch=armv8-a and --with-mode=thumb. >>> >>> Ok for trunk, GCC 5 and 4.9? >>> >>> Han, can you please try this out to see if it solves the problem on your >>> end >>> as well? >>> >>> Thanks, >>> Kyrill >>> >>> 2016-01-21 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >>> >>> PR target/69403 >>> * config/arm/thumb2.md (*thumb2_ior_scc_strict_it): Convert to >>> define_insn_and_split. Ensure operands[1] and operands[0] do not >>> get assigned the same register. >>> >>> 2016-01-21 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >>> >>> PR target/69403 >>> * gcc.c-torture/execute/pr69403.c: New test. >> >> >> >
commit 536a372b7adbb89afa56f61a511ae86e00b7385f Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com> Date: Thu Jan 21 10:15:38 2016 +0000 [ARM] Fix PR target/69403: Bug in thumb2_ior_scc_strict_it pattern diff --git a/gcc/config/arm/thumb2.md b/gcc/config/arm/thumb2.md index 7368d06..9925365 100644 --- a/gcc/config/arm/thumb2.md +++ b/gcc/config/arm/thumb2.md @@ -663,15 +663,27 @@ (define_insn_and_split "*thumb2_ior_scc" (set_attr "type" "multiple")] ) -(define_insn "*thumb2_ior_scc_strict_it" - [(set (match_operand:SI 0 "s_register_operand" "=l,l") +(define_insn_and_split "*thumb2_ior_scc_strict_it" + [(set (match_operand:SI 0 "s_register_operand" "=&r") (ior:SI (match_operator:SI 2 "arm_comparison_operator" [(match_operand 3 "cc_register" "") (const_int 0)]) - (match_operand:SI 1 "s_register_operand" "0,?l")))] + (match_operand:SI 1 "s_register_operand" "r")))] "TARGET_THUMB2 && arm_restrict_it" - "@ - it\\t%d2\;mov%d2\\t%0, #1\;it\\t%d2\;orr%d2\\t%0, %1 - mov\\t%0, #1\;orr\\t%0, %1\;it\\t%D2\;mov%D2\\t%0, %1" + "#" ; orr\\t%0, %1, #1\;it\\t%D2\;mov%D2\\t%0, %1 + "&& reload_completed" + [(set (match_dup 0) (ior:SI (match_dup 1) (const_int 1))) + (cond_exec (match_dup 4) + (set (match_dup 0) (match_dup 1)))] + { + machine_mode mode = GET_MODE (operands[3]); + rtx_code rc = GET_CODE (operands[2]); + + if (mode == CCFPmode || mode == CCFPEmode) + rc = reverse_condition_maybe_unordered (rc); + else + rc = reverse_condition (rc); + operands[4] = gen_rtx_fmt_ee (rc, VOIDmode, operands[3], const0_rtx); + } [(set_attr "conds" "use") (set_attr "length" "8") (set_attr "type" "multiple")] diff --git a/gcc/testsuite/gcc.c-torture/execute/pr69403.c b/gcc/testsuite/gcc.c-torture/execute/pr69403.c new file mode 100644 index 0000000..097d366 --- /dev/null +++ b/gcc/testsuite/gcc.c-torture/execute/pr69403.c @@ -0,0 +1,20 @@ +/* PR target/69403. */ + +int a, b, c; + +__attribute__ ((__noinline__)) int +fn1 () +{ + if ((b | (a != (a & c))) == 1) + __builtin_abort (); + return 0; +} + +int +main (void) +{ + a = 5; + c = 1; + b = 6; + return fn1 (); +}