[PATCH 0/2][GCC] Add one more pattern to RTL if-conversion

[PATCH 0/2][GCC] Add one more pattern to RTL if-conversion

[Mikhail Maltsev]

Hi all!

Currently GCC generates rather bad code for the following test case:

int test(int a)
{
  return a % 2 != 0 ? 4 : 2;
}

The code looks like this:

test:
        andl    $1, %edi
        cmpl    $1, %edi
        sbbl    %eax, %eax
        andl    $-2, %eax
        addl    $4, %eax
        ret

Clang seems to generate optimal code:

test:
        andl    $1, %edi
        leal    2(%rdi,%rdi), %eax
        retq

After applying this series of 2 patches GCC generates:

test:
        movl    %edi, %eax
        andl    $1, %eax
        leal    2(%rax,%rax), %eax
        ret

-- 
Regards,
    Mikhail Maltsev

[PATCH 1/2][GCC] Refactor noce_try_store_flag_constants

[Mikhail Maltsev]

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

This patch refactors 'noce_try_store_flag_constants' a bit to make it easier to
reason about.

The function contains two series of conditions, and each branch of the second
series corresponds to one or two branches of the first series. The patch
introduces a new enumeration strategy_t instead and uses it to select the
correct branch. Also, ISTM that the last 'else' branch is unreachable.

Bootstrapped and regtested on x86_64-linux. OK for trunk?

-- 
Regards,
    Mikhail Maltsev

gcc/ChangeLog:

2016-05-23  Mikhail Maltsev  <maltsevm@gmail.com>

        * ifcvt.c (noce_try_store_flag_constants): Refactor.


[-- Attachment #2: 0001-Refactor-noce_try_store_flag_constants.patch --]
[-- Type: application/x-patch, Size: 6078 bytes --]

Re: [PATCH 1/2][GCC] Refactor noce_try_store_flag_constants

[Bernd Schmidt]

On 05/23/2016 03:58 PM, Mikhail Maltsev wrote:
> This patch refactors 'noce_try_store_flag_constants' a bit to make it easier to
> reason about.
>
> The function contains two series of conditions, and each branch of the second
> series corresponds to one or two branches of the first series. The patch
> introduces a new enumeration strategy_t instead and uses it to select the
> correct branch. Also, ISTM that the last 'else' branch is unreachable.
>
> Bootstrapped and regtested on x86_64-linux. OK for trunk?

Please send patches as text/plain or text/x-patch attachments so they 
can be read and quoted.

+      bool can_reverse
+       = (reversed_comparison_code (if_info->cond, if_info->jump) != 
UNKNOWN);

Parens not needed anymore if the line is split this way. Otherwise OK. 
Maybe the test for common without ST_ADD_FLAG could be moved up a little 
to avoid unnecessary operations.


Bernd

[PATCH 2/2][GCC] Add one more pattern to RTL if-conversion

[Mikhail Maltsev]

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

This patch adds a new if-conversion pattern for the following case:

  if (test) x = A; else x = B;

  A and B are constants, abs(A - B) == 2^N, A != 0, B != 0


Bootstrapped and regtested on x86_64-linux. OK for trunk?

-- 
Regards,
    Mikhail Maltsev

gcc/testsuite/ChangeLog:

2016-05-23  Mikhail Maltsev  <maltsevm@gmail.com>

        * gcc.dg/ifcvt-6.c: New test.


gcc/ChangeLog:

2016-05-23  Mikhail Maltsev  <maltsevm@gmail.com>

        * ifcvt.c (noce_try_store_flag_constants): Add new pattern.

[-- Attachment #2: 0002-Add-ifcvt-pattern.patch --]
[-- Type: application/x-patch, Size: 1943 bytes --]

Re: [PATCH 2/2][GCC] Add one more pattern to RTL if-conversion

[Kyrill Tkachov]

Hi Mikhail,

On 23/05/16 15:00, Mikhail Maltsev wrote:
> This patch adds a new if-conversion pattern for the following case:
>
>    if (test) x = A; else x = B;
>
>    A and B are constants, abs(A - B) == 2^N, A != 0, B != 0
>
>
> Bootstrapped and regtested on x86_64-linux. OK for trunk?
>

@@ -1453,6 +1460,19 @@ noce_try_store_flag_constants (struct noce_if_info *if_info)
  					gen_int_mode (ifalse, mode), if_info->x,
  					0, OPTAB_WIDEN);
  	  break;
+	case ST_SHIFT_ADD_FLAG:
+	  {
+	    /* if (test) x = 5; else x = 1;
+	       =>   x = (test != 0) << 2 + 1;  */
+	    HOST_WIDE_INT diff_log = exact_log2 (abs_hwi (diff));
+	    rtx diff_rtx
+	      = expand_simple_binop (mode, ASHIFT, target, GEN_INT (diff_log),
+				     if_info->x, 0, OPTAB_WIDEN);
+	    target = expand_simple_binop (mode, (diff < 0) ? MINUS : PLUS,
+					  gen_int_mode (ifalse, mode), diff_rtx,
+					  if_info->x, 0, OPTAB_WIDEN);
+	    break;
+	  }

expand_simple_binop may fail. I think you should add a check that diff_rtx is non-NULL
and bail out early if it is.

Kyrill

Re: [PATCH 2/2][GCC] Add one more pattern to RTL if-conversion

[Mikhail Maltsev]

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

On 05/23/2016 05:15 PM, Kyrill Tkachov wrote:
> 
> expand_simple_binop may fail. I think you should add a check that diff_rtx is
> non-NULL
> and bail out early if it is.
> 
Fixed.

-- 
Regards,
    Mikhail Maltsev

[-- Attachment #2: 0002-Add-ifcvt-pattern_v2.patch --]
[-- Type: application/x-patch, Size: 1817 bytes --]

Re: [PATCH 2/2][GCC] Add one more pattern to RTL if-conversion

[Bernd Schmidt]

On 05/24/2016 07:21 PM, Mikhail Maltsev wrote:

+           target = expand_simple_binop (mode, (diff < 0) ? MINUS : PLUS,

Parentheses around (diff < 0) aren't needed. Otherwise I think this is 
also OK.


Bernd