[combine] PR middle-end/123236: Simplify (int)((long long)x >> 4)

Message ID 003f01dc810f$f7d869c0$e7893d40$@nextmovesoftware.com
State New
Headers
Series [combine] PR middle-end/123236: Simplify (int)((long long)x >> 4) |

Commit Message

Roger Sayle Jan. 9, 2026, 2:30 a.m. UTC
  This patch addresses a code quality regression on x86_64 related to
PR 123236.  That original PR (and the related PR 101266) concern tree
level optimizations, where this problem should also be fixed, but it
also reveals a regression in the RTL optimizers.

A motivating test case (on x86_64) is:

unsigned int foo(unsigned int a) {
  unsigned long long t = a;
  return t >> 4;
}

which with -O2 currently generates (2 64-bit shifts):

foo:    movq    %rdi, %rax
        salq    $32, %rax
        shrq    $36, %rax
        ret

with this patch, we now generate (1 32-bit shift):

foo:    movl    %edi, %eax
        shrl    $4, %eax
        ret

which matches what GCC generated prior to GCC 11.
Likewise, for the signed (SIGN_EXTRACT/SIGN_EXTEND) case:

int bar(int a) {
  long long t = a;
  return t >> 4;
}

Before:
bar:    movslq  %edi, %rax
        sarq    $4, %rax
        ret

After:
bar:    movl    %edi, %eax
        sarl    $4, %eax
        ret

The underlying cause of the RTL-level regression is that some
RTL expressions that were previously expressed as {ZERO,SIGN}_EXTEND
are now sometimes represented as the equivalent {ZERO,SIGN}_EXTRACT,
and that not all simplifications of TRUNCATE({ZERO,SIGN}_EXTEND) are
implemented for TRUNCATE({ZERO,SIGN}_EXTRACT).

Thanks to Segher, simplify_rtx does handle some truncations of extracts,
see https://gcc.gnu.org/pipermail/gcc-patches/2016-November/463629.html
but unfortunately this code (and subsequent tweaks) doesn't quite match
the cases that we care about.

Previously:
Trying 6, 7 -> 8:
    6: r103:DI=sign_extend(r105:SI)
      REG_DEAD r105:SI
    7: {r104:DI=r103:DI>>0x4;clobber flags:CC;}
      REG_DEAD r103:DI
      REG_UNUSED flags:CC
    8: r102:SI=r104:DI#0
      REG_DEAD r104:DI
Failed to match this instruction:
(set (subreg:DI (reg:SI 102 [ _4 ]) 0)
    (zero_extend:DI (subreg:SI (sign_extract:DI (reg:SI 105 [ aD.2962 ])
                (const_int 28 [0x1c])
                (const_int 4 [0x4])) 0)))

With the tweaks below, this becomes:
Trying 6, 7 -> 8:
    6: r103:DI=sign_extend(r105:SI)
      REG_DEAD r105:SI
    7: {r104:DI=r103:DI>>0x4;clobber flags:CC;}
      REG_DEAD r103:DI
      REG_UNUSED flags:CC
    8: r102:SI=r104:DI#0
      REG_DEAD r104:DI
Successfully matched this instruction:
(set (reg:SI 102 [ _4 ])
    (ashiftrt:SI (reg:SI 105 [ aD.2962 ])
        (const_int 4 [0x4])))
allowing combination of insns 6, 7 and 8
original costs 4 + 4 + 4 = 12
replacement cost 4
deferring deletion of insn with uid = 7.
deferring deletion of insn with uid = 6.
modifying insn i3     8: {r102:SI=r105:SI>>0x4;clobber flags:CC;}
      REG_UNUSED flags:CC
      REG_DEAD r105:SI
deferring rescan insn with uid = 8.

This requires two minor tweaks.  The first is that the paradoxical
subreg assignment (set (subreg:DI (reg:SI 102) 0) (expr:DI)) can
(sometimes) be simplified to (set (reg:SI 102) (truncate:SI (expr:DI))
especially if the (truncate:SI (expr:DI)) can itself be simplified.
The second is that (truncate:SI (sign_extract:DI (reg:SI 102) x y))
slips through the existing simplify_truncation transformations as the
the {zero,sign}_extract has a different mode to its first operand.

This patch has been tested on x86_64-pc-linux-gnu with make bootstrap
and make -k check, both with and without --target_board=unix{-m32},
with no new failures.  Ok for mainline?

2026-01-09  Roger Sayle  <roger@nextmovesoftware.com>

gcc/ChangeLog
        PR rtl-optimization/123236
        * combine.cc (simplify_set): Attempt to simplify SETs where the
        destination is a low-part paradoxical subreg between scalar integer
        modes.
        * simplify-rtx.cc (simplify_context::simplify_truncation): Handle
        cases where a ZERO_EXTRACT or SIGN_EXTRACT has a different mode
        to (at least as wide as) its first operand.

gcc/testsuite/ChangeLog
        PR rtl-optimization/123236
        * gcc.target/i386/pr123236.c: New test case.


Thanks in advance,
Roger
--
  

Comments

Jeffrey Law May 4, 2026, 4:06 a.m. UTC | #1
On 1/8/2026 7:30 PM, Roger Sayle wrote:
> This patch addresses a code quality regression on x86_64 related to
> PR 123236.  That original PR (and the related PR 101266) concern tree
> level optimizations, where this problem should also be fixed, but it
> also reveals a regression in the RTL optimizers.
>
> A motivating test case (on x86_64) is:
>
> unsigned int foo(unsigned int a) {
>    unsigned long long t = a;
>    return t >> 4;
> }
>
> which with -O2 currently generates (2 64-bit shifts):
>
> foo:    movq    %rdi, %rax
>          salq    $32, %rax
>          shrq    $36, %rax
>          ret
>
> with this patch, we now generate (1 32-bit shift):
>
> foo:    movl    %edi, %eax
>          shrl    $4, %eax
>          ret
>
> which matches what GCC generated prior to GCC 11.
> Likewise, for the signed (SIGN_EXTRACT/SIGN_EXTEND) case:
>
> int bar(int a) {
>    long long t = a;
>    return t >> 4;
> }
>
> Before:
> bar:    movslq  %edi, %rax
>          sarq    $4, %rax
>          ret
>
> After:
> bar:    movl    %edi, %eax
>          sarl    $4, %eax
>          ret
>
> The underlying cause of the RTL-level regression is that some
> RTL expressions that were previously expressed as {ZERO,SIGN}_EXTEND
> are now sometimes represented as the equivalent {ZERO,SIGN}_EXTRACT,
> and that not all simplifications of TRUNCATE({ZERO,SIGN}_EXTEND) are
> implemented for TRUNCATE({ZERO,SIGN}_EXTRACT).
>
> Thanks to Segher, simplify_rtx does handle some truncations of extracts,
> see https://gcc.gnu.org/pipermail/gcc-patches/2016-November/463629.html
> but unfortunately this code (and subsequent tweaks) doesn't quite match
> the cases that we care about.
>
> Previously:
> Trying 6, 7 -> 8:
>      6: r103:DI=sign_extend(r105:SI)
>        REG_DEAD r105:SI
>      7: {r104:DI=r103:DI>>0x4;clobber flags:CC;}
>        REG_DEAD r103:DI
>        REG_UNUSED flags:CC
>      8: r102:SI=r104:DI#0
>        REG_DEAD r104:DI
> Failed to match this instruction:
> (set (subreg:DI (reg:SI 102 [ _4 ]) 0)
>      (zero_extend:DI (subreg:SI (sign_extract:DI (reg:SI 105 [ aD.2962 ])
>                  (const_int 28 [0x1c])
>                  (const_int 4 [0x4])) 0)))
>
> With the tweaks below, this becomes:
> Trying 6, 7 -> 8:
>      6: r103:DI=sign_extend(r105:SI)
>        REG_DEAD r105:SI
>      7: {r104:DI=r103:DI>>0x4;clobber flags:CC;}
>        REG_DEAD r103:DI
>        REG_UNUSED flags:CC
>      8: r102:SI=r104:DI#0
>        REG_DEAD r104:DI
> Successfully matched this instruction:
> (set (reg:SI 102 [ _4 ])
>      (ashiftrt:SI (reg:SI 105 [ aD.2962 ])
>          (const_int 4 [0x4])))
> allowing combination of insns 6, 7 and 8
> original costs 4 + 4 + 4 = 12
> replacement cost 4
> deferring deletion of insn with uid = 7.
> deferring deletion of insn with uid = 6.
> modifying insn i3     8: {r102:SI=r105:SI>>0x4;clobber flags:CC;}
>        REG_UNUSED flags:CC
>        REG_DEAD r105:SI
> deferring rescan insn with uid = 8.
>
> This requires two minor tweaks.  The first is that the paradoxical
> subreg assignment (set (subreg:DI (reg:SI 102) 0) (expr:DI)) can
> (sometimes) be simplified to (set (reg:SI 102) (truncate:SI (expr:DI))
> especially if the (truncate:SI (expr:DI)) can itself be simplified.
> The second is that (truncate:SI (sign_extract:DI (reg:SI 102) x y))
> slips through the existing simplify_truncation transformations as the
> the {zero,sign}_extract has a different mode to its first operand.
>
> This patch has been tested on x86_64-pc-linux-gnu with make bootstrap
> and make -k check, both with and without --target_board=unix{-m32},
> with no new failures.  Ok for mainline?
>
> 2026-01-09  Roger Sayle  <roger@nextmovesoftware.com>
>
> gcc/ChangeLog
>          PR rtl-optimization/123236
>          * combine.cc (simplify_set): Attempt to simplify SETs where the
>          destination is a low-part paradoxical subreg between scalar integer
>          modes.
>          * simplify-rtx.cc (simplify_context::simplify_truncation): Handle
>          cases where a ZERO_EXTRACT or SIGN_EXTRACT has a different mode
>          to (at least as wide as) its first operand.
>
> gcc/testsuite/ChangeLog
>          PR rtl-optimization/123236
>          * gcc.target/i386/pr123236.c: New test case.
So in simplify_truncation you dropped the REG_P (XEXP (op, 0)) check.  
Was that intentional?  I guess since you don't ultimately change the 
mode of the truncated object that's probably OK.  So as long as it was 
intentional, I won't object to it.

So if the removal was intentional, then OK as-is.  If it was a goof, 
then OK with the check restored.

jeff
  
Richard Sandiford May 4, 2026, 4:46 p.m. UTC | #2
Jeffrey Law <jefflaw@qti.qualcomm.com> writes:
> On 1/8/2026 7:30 PM, Roger Sayle wrote:
>> This patch addresses a code quality regression on x86_64 related to
>> PR 123236.  That original PR (and the related PR 101266) concern tree
>> level optimizations, where this problem should also be fixed, but it
>> also reveals a regression in the RTL optimizers.
>>
>> A motivating test case (on x86_64) is:
>>
>> unsigned int foo(unsigned int a) {
>>    unsigned long long t = a;
>>    return t >> 4;
>> }
>>
>> which with -O2 currently generates (2 64-bit shifts):
>>
>> foo:    movq    %rdi, %rax
>>          salq    $32, %rax
>>          shrq    $36, %rax
>>          ret
>>
>> with this patch, we now generate (1 32-bit shift):
>>
>> foo:    movl    %edi, %eax
>>          shrl    $4, %eax
>>          ret
>>
>> which matches what GCC generated prior to GCC 11.
>> Likewise, for the signed (SIGN_EXTRACT/SIGN_EXTEND) case:
>>
>> int bar(int a) {
>>    long long t = a;
>>    return t >> 4;
>> }
>>
>> Before:
>> bar:    movslq  %edi, %rax
>>          sarq    $4, %rax
>>          ret
>>
>> After:
>> bar:    movl    %edi, %eax
>>          sarl    $4, %eax
>>          ret
>>
>> The underlying cause of the RTL-level regression is that some
>> RTL expressions that were previously expressed as {ZERO,SIGN}_EXTEND
>> are now sometimes represented as the equivalent {ZERO,SIGN}_EXTRACT,
>> and that not all simplifications of TRUNCATE({ZERO,SIGN}_EXTEND) are
>> implemented for TRUNCATE({ZERO,SIGN}_EXTRACT).
>>
>> Thanks to Segher, simplify_rtx does handle some truncations of extracts,
>> see https://gcc.gnu.org/pipermail/gcc-patches/2016-November/463629.html
>> but unfortunately this code (and subsequent tweaks) doesn't quite match
>> the cases that we care about.
>>
>> Previously:
>> Trying 6, 7 -> 8:
>>      6: r103:DI=sign_extend(r105:SI)
>>        REG_DEAD r105:SI
>>      7: {r104:DI=r103:DI>>0x4;clobber flags:CC;}
>>        REG_DEAD r103:DI
>>        REG_UNUSED flags:CC
>>      8: r102:SI=r104:DI#0
>>        REG_DEAD r104:DI
>> Failed to match this instruction:
>> (set (subreg:DI (reg:SI 102 [ _4 ]) 0)
>>      (zero_extend:DI (subreg:SI (sign_extract:DI (reg:SI 105 [ aD.2962 ])
>>                  (const_int 28 [0x1c])
>>                  (const_int 4 [0x4])) 0)))
>>
>> With the tweaks below, this becomes:
>> Trying 6, 7 -> 8:
>>      6: r103:DI=sign_extend(r105:SI)
>>        REG_DEAD r105:SI
>>      7: {r104:DI=r103:DI>>0x4;clobber flags:CC;}
>>        REG_DEAD r103:DI
>>        REG_UNUSED flags:CC
>>      8: r102:SI=r104:DI#0
>>        REG_DEAD r104:DI
>> Successfully matched this instruction:
>> (set (reg:SI 102 [ _4 ])
>>      (ashiftrt:SI (reg:SI 105 [ aD.2962 ])
>>          (const_int 4 [0x4])))
>> allowing combination of insns 6, 7 and 8
>> original costs 4 + 4 + 4 = 12
>> replacement cost 4
>> deferring deletion of insn with uid = 7.
>> deferring deletion of insn with uid = 6.
>> modifying insn i3     8: {r102:SI=r105:SI>>0x4;clobber flags:CC;}
>>        REG_UNUSED flags:CC
>>        REG_DEAD r105:SI
>> deferring rescan insn with uid = 8.
>>
>> This requires two minor tweaks.  The first is that the paradoxical
>> subreg assignment (set (subreg:DI (reg:SI 102) 0) (expr:DI)) can
>> (sometimes) be simplified to (set (reg:SI 102) (truncate:SI (expr:DI))
>> especially if the (truncate:SI (expr:DI)) can itself be simplified.
>> The second is that (truncate:SI (sign_extract:DI (reg:SI 102) x y))
>> slips through the existing simplify_truncation transformations as the
>> the {zero,sign}_extract has a different mode to its first operand.
>>
>> This patch has been tested on x86_64-pc-linux-gnu with make bootstrap
>> and make -k check, both with and without --target_board=unix{-m32},
>> with no new failures.  Ok for mainline?
>>
>> 2026-01-09  Roger Sayle  <roger@nextmovesoftware.com>
>>
>> gcc/ChangeLog
>>          PR rtl-optimization/123236
>>          * combine.cc (simplify_set): Attempt to simplify SETs where the
>>          destination is a low-part paradoxical subreg between scalar integer
>>          modes.
>>          * simplify-rtx.cc (simplify_context::simplify_truncation): Handle
>>          cases where a ZERO_EXTRACT or SIGN_EXTRACT has a different mode
>>          to (at least as wide as) its first operand.
>>
>> gcc/testsuite/ChangeLog
>>          PR rtl-optimization/123236
>>          * gcc.target/i386/pr123236.c: New test case.
> So in simplify_truncation you dropped the REG_P (XEXP (op, 0)) check.  
> Was that intentional?  I guess since you don't ultimately change the 
> mode of the truncated object that's probably OK.  So as long as it was 
> intentional, I won't object to it.

Also, I wonder how friendly this would be on RISC targets.  The use
of simplify_gen_unary...

> +  /* If we have (set (subreg:M (reg:N DEST) SRC) with M wider than N,
> +     we have a paradoxical subreg destination, such as created by the
> +     clause above, check if we can simplify (subreg:N SRC) to eliminate
> +     the (paradoxical) subreg.  */
> +  else if (GET_CODE (dest) == SUBREG
> +	   && subreg_lowpart_p (dest)
> +	   && paradoxical_subreg_p (dest)
> +	   && SCALAR_INT_MODE_P (mode)
> +	   && SCALAR_INT_MODE_P (GET_MODE (SUBREG_REG (dest))))
> +    {
> +      rtx tmp = simplify_gen_unary (TRUNCATE, GET_MODE (SUBREG_REG (dest)),
> +				    src, mode);

...here means that we'd narrow the operation even if no simplification of
the rhs is possible.  So, for example, I can imagine this trying to create
a QImode operation on targets that only provide SImode or DImode operations.

This seems like something that would be worth trying if a recog of the
original pattern fails, but maybe isn't something that we should commit
to up-front, before any recog has been attempted.

(BTW, the simplify_lowpart_p check is redundant with the
paradoxical_subreg_p check.)

Thanks,
Richard

> +      if (tmp)
> +	{
> +	  SUBST (SET_DEST (x), SUBREG_REG (dest));
> +	  SUBST (SET_SRC (x), tmp);
> +	  dest = SET_DEST (x);
> +	  src = SET_SRC (x);
> +	}
> +    }
> +
  
Jeffrey Law May 4, 2026, 5:10 p.m. UTC | #3
On 5/4/2026 10:46 AM, Richard Sandiford wrote:
> WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros.
>
> Jeffrey Law <jefflaw@qti.qualcomm.com> writes:
>> On 1/8/2026 7:30 PM, Roger Sayle wrote:
>>> This patch addresses a code quality regression on x86_64 related to
>>> PR 123236.  That original PR (and the related PR 101266) concern tree
>>> level optimizations, where this problem should also be fixed, but it
>>> also reveals a regression in the RTL optimizers.
>>>
>>> A motivating test case (on x86_64) is:
>>>
>>> unsigned int foo(unsigned int a) {
>>>     unsigned long long t = a;
>>>     return t >> 4;
>>> }
>>>
>>> which with -O2 currently generates (2 64-bit shifts):
>>>
>>> foo:    movq    %rdi, %rax
>>>           salq    $32, %rax
>>>           shrq    $36, %rax
>>>           ret
>>>
>>> with this patch, we now generate (1 32-bit shift):
>>>
>>> foo:    movl    %edi, %eax
>>>           shrl    $4, %eax
>>>           ret
>>>
>>> which matches what GCC generated prior to GCC 11.
>>> Likewise, for the signed (SIGN_EXTRACT/SIGN_EXTEND) case:
>>>
>>> int bar(int a) {
>>>     long long t = a;
>>>     return t >> 4;
>>> }
>>>
>>> Before:
>>> bar:    movslq  %edi, %rax
>>>           sarq    $4, %rax
>>>           ret
>>>
>>> After:
>>> bar:    movl    %edi, %eax
>>>           sarl    $4, %eax
>>>           ret
>>>
>>> The underlying cause of the RTL-level regression is that some
>>> RTL expressions that were previously expressed as {ZERO,SIGN}_EXTEND
>>> are now sometimes represented as the equivalent {ZERO,SIGN}_EXTRACT,
>>> and that not all simplifications of TRUNCATE({ZERO,SIGN}_EXTEND) are
>>> implemented for TRUNCATE({ZERO,SIGN}_EXTRACT).
>>>
>>> Thanks to Segher, simplify_rtx does handle some truncations of extracts,
>>> see https://gcc.gnu.org/pipermail/gcc-patches/2016-November/463629.html
>>> but unfortunately this code (and subsequent tweaks) doesn't quite match
>>> the cases that we care about.
>>>
>>> Previously:
>>> Trying 6, 7 -> 8:
>>>       6: r103:DI=sign_extend(r105:SI)
>>>         REG_DEAD r105:SI
>>>       7: {r104:DI=r103:DI>>0x4;clobber flags:CC;}
>>>         REG_DEAD r103:DI
>>>         REG_UNUSED flags:CC
>>>       8: r102:SI=r104:DI#0
>>>         REG_DEAD r104:DI
>>> Failed to match this instruction:
>>> (set (subreg:DI (reg:SI 102 [ _4 ]) 0)
>>>       (zero_extend:DI (subreg:SI (sign_extract:DI (reg:SI 105 [ aD.2962 ])
>>>                   (const_int 28 [0x1c])
>>>                   (const_int 4 [0x4])) 0)))
>>>
>>> With the tweaks below, this becomes:
>>> Trying 6, 7 -> 8:
>>>       6: r103:DI=sign_extend(r105:SI)
>>>         REG_DEAD r105:SI
>>>       7: {r104:DI=r103:DI>>0x4;clobber flags:CC;}
>>>         REG_DEAD r103:DI
>>>         REG_UNUSED flags:CC
>>>       8: r102:SI=r104:DI#0
>>>         REG_DEAD r104:DI
>>> Successfully matched this instruction:
>>> (set (reg:SI 102 [ _4 ])
>>>       (ashiftrt:SI (reg:SI 105 [ aD.2962 ])
>>>           (const_int 4 [0x4])))
>>> allowing combination of insns 6, 7 and 8
>>> original costs 4 + 4 + 4 = 12
>>> replacement cost 4
>>> deferring deletion of insn with uid = 7.
>>> deferring deletion of insn with uid = 6.
>>> modifying insn i3     8: {r102:SI=r105:SI>>0x4;clobber flags:CC;}
>>>         REG_UNUSED flags:CC
>>>         REG_DEAD r105:SI
>>> deferring rescan insn with uid = 8.
>>>
>>> This requires two minor tweaks.  The first is that the paradoxical
>>> subreg assignment (set (subreg:DI (reg:SI 102) 0) (expr:DI)) can
>>> (sometimes) be simplified to (set (reg:SI 102) (truncate:SI (expr:DI))
>>> especially if the (truncate:SI (expr:DI)) can itself be simplified.
>>> The second is that (truncate:SI (sign_extract:DI (reg:SI 102) x y))
>>> slips through the existing simplify_truncation transformations as the
>>> the {zero,sign}_extract has a different mode to its first operand.
>>>
>>> This patch has been tested on x86_64-pc-linux-gnu with make bootstrap
>>> and make -k check, both with and without --target_board=unix{-m32},
>>> with no new failures.  Ok for mainline?
>>>
>>> 2026-01-09  Roger Sayle  <roger@nextmovesoftware.com>
>>>
>>> gcc/ChangeLog
>>>           PR rtl-optimization/123236
>>>           * combine.cc (simplify_set): Attempt to simplify SETs where the
>>>           destination is a low-part paradoxical subreg between scalar integer
>>>           modes.
>>>           * simplify-rtx.cc (simplify_context::simplify_truncation): Handle
>>>           cases where a ZERO_EXTRACT or SIGN_EXTRACT has a different mode
>>>           to (at least as wide as) its first operand.
>>>
>>> gcc/testsuite/ChangeLog
>>>           PR rtl-optimization/123236
>>>           * gcc.target/i386/pr123236.c: New test case.
>> So in simplify_truncation you dropped the REG_P (XEXP (op, 0)) check.
>> Was that intentional?  I guess since you don't ultimately change the
>> mode of the truncated object that's probably OK.  So as long as it was
>> intentional, I won't object to it.
> Also, I wonder how friendly this would be on RISC targets.  The use
> of simplify_gen_unary...
Are you referring to simplify_gen_unary failing and returning NULL or is 
the resultant code we generate RISC friendly?  For the latter, we just 
potentially miss an optimization, the former could be an ICE/segfault 
scenario.

Jeff
  
Richard Sandiford May 4, 2026, 5:16 p.m. UTC | #4
Jeffrey Law <jefflaw@qti.qualcomm.com> writes:
> On 5/4/2026 10:46 AM, Richard Sandiford wrote:
>> Jeffrey Law <jefflaw@qti.qualcomm.com> writes:
>>> On 1/8/2026 7:30 PM, Roger Sayle wrote:
>>>> This patch addresses a code quality regression on x86_64 related to
>>>> PR 123236.  That original PR (and the related PR 101266) concern tree
>>>> level optimizations, where this problem should also be fixed, but it
>>>> also reveals a regression in the RTL optimizers.
>>>>
>>>> A motivating test case (on x86_64) is:
>>>>
>>>> unsigned int foo(unsigned int a) {
>>>>     unsigned long long t = a;
>>>>     return t >> 4;
>>>> }
>>>>
>>>> which with -O2 currently generates (2 64-bit shifts):
>>>>
>>>> foo:    movq    %rdi, %rax
>>>>           salq    $32, %rax
>>>>           shrq    $36, %rax
>>>>           ret
>>>>
>>>> with this patch, we now generate (1 32-bit shift):
>>>>
>>>> foo:    movl    %edi, %eax
>>>>           shrl    $4, %eax
>>>>           ret
>>>>
>>>> which matches what GCC generated prior to GCC 11.
>>>> Likewise, for the signed (SIGN_EXTRACT/SIGN_EXTEND) case:
>>>>
>>>> int bar(int a) {
>>>>     long long t = a;
>>>>     return t >> 4;
>>>> }
>>>>
>>>> Before:
>>>> bar:    movslq  %edi, %rax
>>>>           sarq    $4, %rax
>>>>           ret
>>>>
>>>> After:
>>>> bar:    movl    %edi, %eax
>>>>           sarl    $4, %eax
>>>>           ret
>>>>
>>>> The underlying cause of the RTL-level regression is that some
>>>> RTL expressions that were previously expressed as {ZERO,SIGN}_EXTEND
>>>> are now sometimes represented as the equivalent {ZERO,SIGN}_EXTRACT,
>>>> and that not all simplifications of TRUNCATE({ZERO,SIGN}_EXTEND) are
>>>> implemented for TRUNCATE({ZERO,SIGN}_EXTRACT).
>>>>
>>>> Thanks to Segher, simplify_rtx does handle some truncations of extracts,
>>>> see https://gcc.gnu.org/pipermail/gcc-patches/2016-November/463629.html
>>>> but unfortunately this code (and subsequent tweaks) doesn't quite match
>>>> the cases that we care about.
>>>>
>>>> Previously:
>>>> Trying 6, 7 -> 8:
>>>>       6: r103:DI=sign_extend(r105:SI)
>>>>         REG_DEAD r105:SI
>>>>       7: {r104:DI=r103:DI>>0x4;clobber flags:CC;}
>>>>         REG_DEAD r103:DI
>>>>         REG_UNUSED flags:CC
>>>>       8: r102:SI=r104:DI#0
>>>>         REG_DEAD r104:DI
>>>> Failed to match this instruction:
>>>> (set (subreg:DI (reg:SI 102 [ _4 ]) 0)
>>>>       (zero_extend:DI (subreg:SI (sign_extract:DI (reg:SI 105 [ aD.2962 ])
>>>>                   (const_int 28 [0x1c])
>>>>                   (const_int 4 [0x4])) 0)))
>>>>
>>>> With the tweaks below, this becomes:
>>>> Trying 6, 7 -> 8:
>>>>       6: r103:DI=sign_extend(r105:SI)
>>>>         REG_DEAD r105:SI
>>>>       7: {r104:DI=r103:DI>>0x4;clobber flags:CC;}
>>>>         REG_DEAD r103:DI
>>>>         REG_UNUSED flags:CC
>>>>       8: r102:SI=r104:DI#0
>>>>         REG_DEAD r104:DI
>>>> Successfully matched this instruction:
>>>> (set (reg:SI 102 [ _4 ])
>>>>       (ashiftrt:SI (reg:SI 105 [ aD.2962 ])
>>>>           (const_int 4 [0x4])))
>>>> allowing combination of insns 6, 7 and 8
>>>> original costs 4 + 4 + 4 = 12
>>>> replacement cost 4
>>>> deferring deletion of insn with uid = 7.
>>>> deferring deletion of insn with uid = 6.
>>>> modifying insn i3     8: {r102:SI=r105:SI>>0x4;clobber flags:CC;}
>>>>         REG_UNUSED flags:CC
>>>>         REG_DEAD r105:SI
>>>> deferring rescan insn with uid = 8.
>>>>
>>>> This requires two minor tweaks.  The first is that the paradoxical
>>>> subreg assignment (set (subreg:DI (reg:SI 102) 0) (expr:DI)) can
>>>> (sometimes) be simplified to (set (reg:SI 102) (truncate:SI (expr:DI))
>>>> especially if the (truncate:SI (expr:DI)) can itself be simplified.
>>>> The second is that (truncate:SI (sign_extract:DI (reg:SI 102) x y))
>>>> slips through the existing simplify_truncation transformations as the
>>>> the {zero,sign}_extract has a different mode to its first operand.
>>>>
>>>> This patch has been tested on x86_64-pc-linux-gnu with make bootstrap
>>>> and make -k check, both with and without --target_board=unix{-m32},
>>>> with no new failures.  Ok for mainline?
>>>>
>>>> 2026-01-09  Roger Sayle  <roger@nextmovesoftware.com>
>>>>
>>>> gcc/ChangeLog
>>>>           PR rtl-optimization/123236
>>>>           * combine.cc (simplify_set): Attempt to simplify SETs where the
>>>>           destination is a low-part paradoxical subreg between scalar integer
>>>>           modes.
>>>>           * simplify-rtx.cc (simplify_context::simplify_truncation): Handle
>>>>           cases where a ZERO_EXTRACT or SIGN_EXTRACT has a different mode
>>>>           to (at least as wide as) its first operand.
>>>>
>>>> gcc/testsuite/ChangeLog
>>>>           PR rtl-optimization/123236
>>>>           * gcc.target/i386/pr123236.c: New test case.
>>> So in simplify_truncation you dropped the REG_P (XEXP (op, 0)) check.
>>> Was that intentional?  I guess since you don't ultimately change the
>>> mode of the truncated object that's probably OK.  So as long as it was
>>> intentional, I won't object to it.
>> Also, I wonder how friendly this would be on RISC targets.  The use
>> of simplify_gen_unary...
> Are you referring to simplify_gen_unary failing and returning NULL or is 
> the resultant code we generate RISC friendly?  For the latter, we just 
> potentially miss an optimization, the former could be an ICE/segfault 
> scenario.

Yeah, it was the latter.  If the code had used simplify_unary_operation
instead of simplify_gen_unary, it would at least fire only when a
simplification is possible.  But as written, it looks like it
effectively eliminates all assignments to paradoxical subregs,
provided that both modes are integral.

So I agree that it's only a missed optimisation concern rather than a
correctness concern.  But it doesn't feel like a natural canonicalisation
for RISC.

Thanks,
Richard
  
Roger Sayle May 4, 2026, 10:52 p.m. UTC | #5
Hi Richard,
The same class of bug is also present on aarch64, as an example of a RISC
target, but there it presents differently in the RTL, due to the internal
representations making less use of SUBREG.

Consider the four functions:

int sfoo(int a) {
  long long t = a;
  return t >> 4;
}

unsigned int ufoo(unsigned int a) {
  unsigned long long t = a;
  return t >> 4;
}

long long sbar(long long x)
{
    __int128 t = x;
    return t >> 4;
}

unsigned long long ubar(unsigned long long x)
{
    unsigned __int128 t = x;
    return t >> 4;
}

With -O2, GCC on aarch64 currently generates:

sfoo:
        sbfx    x0, x0, 4, 28
        ret
ufoo:
        lsr     w0, w0, 4
        ret
sbar:
        asr     x1, x0, 63
        extr    x0, x1, x0, 4
        ret
ubar:
        lsr     x0, x0, 4
        ret

Whereas current clang/LLVM generates (the hoped for):

sfoo:
        asr     w0, w0, #4
        ret

ufoo:
        lsr     w0, w0, #4
        ret

sbar:
        asr     x0, x0, #4
        ret

ubar:
        lsr     x0, x0, #4
        ret


So the motivation for the patch is independent of the target,
it's just that some (RISC) backends don’t suffer SUBREG headaches
to the same extent as others (such as x86 and AVR).

Roger
--

> -----Original Message-----
> From: Richard Sandiford <rdsandiford@googlemail.com>
> Sent: 04 May 2026 17:46
> To: Jeffrey Law <jefflaw@qti.qualcomm.com>
> Cc: Roger Sayle <roger@nextmovesoftware.com>; 'GCC Patches' <gcc-
> patches@gcc.gnu.org>; 'Segher Boessenkool' <segher@kernel.crashing.org>
> Subject: Re: [combine] PR middle-end/123236: Simplify (int)((long long)x >> 4)
> 
> Jeffrey Law <jefflaw@qti.qualcomm.com> writes:
> > On 1/8/2026 7:30 PM, Roger Sayle wrote:
> >> This patch addresses a code quality regression on x86_64 related to
> >> PR 123236.  That original PR (and the related PR 101266) concern tree
> >> level optimizations, where this problem should also be fixed, but it
> >> also reveals a regression in the RTL optimizers.
> >>
> >> A motivating test case (on x86_64) is:
> >>
> >> unsigned int foo(unsigned int a) {
> >>    unsigned long long t = a;
> >>    return t >> 4;
> >> }
> >>
> >> which with -O2 currently generates (2 64-bit shifts):
> >>
> >> foo:    movq    %rdi, %rax
> >>          salq    $32, %rax
> >>          shrq    $36, %rax
> >>          ret
> >>
> >> with this patch, we now generate (1 32-bit shift):
> >>
> >> foo:    movl    %edi, %eax
> >>          shrl    $4, %eax
> >>          ret
> >>
> >> which matches what GCC generated prior to GCC 11.
> >> Likewise, for the signed (SIGN_EXTRACT/SIGN_EXTEND) case:
> >>
> >> int bar(int a) {
> >>    long long t = a;
> >>    return t >> 4;
> >> }
> >>
> >> Before:
> >> bar:    movslq  %edi, %rax
> >>          sarq    $4, %rax
> >>          ret
> >>
> >> After:
> >> bar:    movl    %edi, %eax
> >>          sarl    $4, %eax
> >>          ret
> >>
> >> The underlying cause of the RTL-level regression is that some RTL
> >> expressions that were previously expressed as {ZERO,SIGN}_EXTEND are
> >> now sometimes represented as the equivalent {ZERO,SIGN}_EXTRACT, and
> >> that not all simplifications of TRUNCATE({ZERO,SIGN}_EXTEND) are
> >> implemented for TRUNCATE({ZERO,SIGN}_EXTRACT).
> >>
> >> Thanks to Segher, simplify_rtx does handle some truncations of
> >> extracts, see
> >> https://gcc.gnu.org/pipermail/gcc-patches/2016-November/463629.html
> >> but unfortunately this code (and subsequent tweaks) doesn't quite
> >> match the cases that we care about.
> >>
> >> Previously:
> >> Trying 6, 7 -> 8:
> >>      6: r103:DI=sign_extend(r105:SI)
> >>        REG_DEAD r105:SI
> >>      7: {r104:DI=r103:DI>>0x4;clobber flags:CC;}
> >>        REG_DEAD r103:DI
> >>        REG_UNUSED flags:CC
> >>      8: r102:SI=r104:DI#0
> >>        REG_DEAD r104:DI
> >> Failed to match this instruction:
> >> (set (subreg:DI (reg:SI 102 [ _4 ]) 0)
> >>      (zero_extend:DI (subreg:SI (sign_extract:DI (reg:SI 105 [ aD.2962 ])
> >>                  (const_int 28 [0x1c])
> >>                  (const_int 4 [0x4])) 0)))
> >>
> >> With the tweaks below, this becomes:
> >> Trying 6, 7 -> 8:
> >>      6: r103:DI=sign_extend(r105:SI)
> >>        REG_DEAD r105:SI
> >>      7: {r104:DI=r103:DI>>0x4;clobber flags:CC;}
> >>        REG_DEAD r103:DI
> >>        REG_UNUSED flags:CC
> >>      8: r102:SI=r104:DI#0
> >>        REG_DEAD r104:DI
> >> Successfully matched this instruction:
> >> (set (reg:SI 102 [ _4 ])
> >>      (ashiftrt:SI (reg:SI 105 [ aD.2962 ])
> >>          (const_int 4 [0x4])))
> >> allowing combination of insns 6, 7 and 8 original costs 4 + 4 + 4 =
> >> 12 replacement cost 4 deferring deletion of insn with uid = 7.
> >> deferring deletion of insn with uid = 6.
> >> modifying insn i3     8: {r102:SI=r105:SI>>0x4;clobber flags:CC;}
> >>        REG_UNUSED flags:CC
> >>        REG_DEAD r105:SI
> >> deferring rescan insn with uid = 8.
> >>
> >> This requires two minor tweaks.  The first is that the paradoxical
> >> subreg assignment (set (subreg:DI (reg:SI 102) 0) (expr:DI)) can
> >> (sometimes) be simplified to (set (reg:SI 102) (truncate:SI
> >> (expr:DI)) especially if the (truncate:SI (expr:DI)) can itself be simplified.
> >> The second is that (truncate:SI (sign_extract:DI (reg:SI 102) x y))
> >> slips through the existing simplify_truncation transformations as the
> >> the {zero,sign}_extract has a different mode to its first operand.
> >>
> >> This patch has been tested on x86_64-pc-linux-gnu with make bootstrap
> >> and make -k check, both with and without --target_board=unix{-m32},
> >> with no new failures.  Ok for mainline?
> >>
> >> 2026-01-09  Roger Sayle  <roger@nextmovesoftware.com>
> >>
> >> gcc/ChangeLog
> >>          PR rtl-optimization/123236
> >>          * combine.cc (simplify_set): Attempt to simplify SETs where the
> >>          destination is a low-part paradoxical subreg between scalar integer
> >>          modes.
> >>          * simplify-rtx.cc (simplify_context::simplify_truncation): Handle
> >>          cases where a ZERO_EXTRACT or SIGN_EXTRACT has a different mode
> >>          to (at least as wide as) its first operand.
> >>
> >> gcc/testsuite/ChangeLog
> >>          PR rtl-optimization/123236
> >>          * gcc.target/i386/pr123236.c: New test case.
> > So in simplify_truncation you dropped the REG_P (XEXP (op, 0)) check.
> > Was that intentional?  I guess since you don't ultimately change the
> > mode of the truncated object that's probably OK.  So as long as it was
> > intentional, I won't object to it.
> 
> Also, I wonder how friendly this would be on RISC targets.  The use of
> simplify_gen_unary...
> 
> > +  /* If we have (set (subreg:M (reg:N DEST) SRC) with M wider than N,
> > +     we have a paradoxical subreg destination, such as created by the
> > +     clause above, check if we can simplify (subreg:N SRC) to eliminate
> > +     the (paradoxical) subreg.  */
> > +  else if (GET_CODE (dest) == SUBREG
> > +	   && subreg_lowpart_p (dest)
> > +	   && paradoxical_subreg_p (dest)
> > +	   && SCALAR_INT_MODE_P (mode)
> > +	   && SCALAR_INT_MODE_P (GET_MODE (SUBREG_REG (dest))))
> > +    {
> > +      rtx tmp = simplify_gen_unary (TRUNCATE, GET_MODE (SUBREG_REG
> (dest)),
> > +				    src, mode);
> 
> ...here means that we'd narrow the operation even if no simplification of the rhs
> is possible.  So, for example, I can imagine this trying to create a QImode
> operation on targets that only provide SImode or DImode operations.
> 
> This seems like something that would be worth trying if a recog of the original
> pattern fails, but maybe isn't something that we should commit to up-front, before
> any recog has been attempted.
> 
> (BTW, the simplify_lowpart_p check is redundant with the paradoxical_subreg_p
> check.)
> 
> Thanks,
> Richard
> 
> > +      if (tmp)
> > +	{
> > +	  SUBST (SET_DEST (x), SUBREG_REG (dest));
> > +	  SUBST (SET_SRC (x), tmp);
> > +	  dest = SET_DEST (x);
> > +	  src = SET_SRC (x);
> > +	}
> > +    }
> > +
  
Roger Sayle May 4, 2026, 11:32 p.m. UTC | #6
Hi Jeff,
Thanks for the review.

Jeff Law wrote:
> On 1/8/2026 7:30 PM, Roger Sayle wrote:
> > This patch addresses a code quality regression on x86_64 related to PR
> > 123236.  That original PR (and the related PR 101266) concern tree
> > level optimizations, where this problem should also be fixed, but it
> > also reveals a regression in the RTL optimizers.
> >
> > A motivating test case (on x86_64) is:
> >
> > unsigned int foo(unsigned int a) {
> >    unsigned long long t = a;
> >    return t >> 4;
> > }
> >
> > which with -O2 currently generates (2 64-bit shifts):
> >
> > foo:    movq    %rdi, %rax
> >          salq    $32, %rax
> >          shrq    $36, %rax
> >          ret
> >
> > with this patch, we now generate (1 32-bit shift):
> >
> > foo:    movl    %edi, %eax
> >          shrl    $4, %eax
> >          ret
> >
> > which matches what GCC generated prior to GCC 11.
> > Likewise, for the signed (SIGN_EXTRACT/SIGN_EXTEND) case:
> >
> > int bar(int a) {
> >    long long t = a;
> >    return t >> 4;
> > }
> >
> > Before:
> > bar:    movslq  %edi, %rax
> >          sarq    $4, %rax
> >          ret
> >
> > After:
> > bar:    movl    %edi, %eax
> >          sarl    $4, %eax
> >          ret
> >
> > The underlying cause of the RTL-level regression is that some RTL
> > expressions that were previously expressed as {ZERO,SIGN}_EXTEND are
> > now sometimes represented as the equivalent {ZERO,SIGN}_EXTRACT, and
> > that not all simplifications of TRUNCATE({ZERO,SIGN}_EXTEND) are
> > implemented for TRUNCATE({ZERO,SIGN}_EXTRACT).
> >
> > Thanks to Segher, simplify_rtx does handle some truncations of
> > extracts, see
> > https://gcc.gnu.org/pipermail/gcc-patches/2016-November/463629.html
> > but unfortunately this code (and subsequent tweaks) doesn't quite
> > match the cases that we care about.
> >
> > Previously:
> > Trying 6, 7 -> 8:
> >      6: r103:DI=sign_extend(r105:SI)
> >        REG_DEAD r105:SI
> >      7: {r104:DI=r103:DI>>0x4;clobber flags:CC;}
> >        REG_DEAD r103:DI
> >        REG_UNUSED flags:CC
> >      8: r102:SI=r104:DI#0
> >        REG_DEAD r104:DI
> > Failed to match this instruction:
> > (set (subreg:DI (reg:SI 102 [ _4 ]) 0)
> >      (zero_extend:DI (subreg:SI (sign_extract:DI (reg:SI 105 [ aD.2962 ])
> >                  (const_int 28 [0x1c])
> >                  (const_int 4 [0x4])) 0)))
> >
> > With the tweaks below, this becomes:
> > Trying 6, 7 -> 8:
> >      6: r103:DI=sign_extend(r105:SI)
> >        REG_DEAD r105:SI
> >      7: {r104:DI=r103:DI>>0x4;clobber flags:CC;}
> >        REG_DEAD r103:DI
> >        REG_UNUSED flags:CC
> >      8: r102:SI=r104:DI#0
> >        REG_DEAD r104:DI
> > Successfully matched this instruction:
> > (set (reg:SI 102 [ _4 ])
> >      (ashiftrt:SI (reg:SI 105 [ aD.2962 ])
> >          (const_int 4 [0x4])))
> > allowing combination of insns 6, 7 and 8 original costs 4 + 4 + 4 = 12
> > replacement cost 4 deferring deletion of insn with uid = 7.
> > deferring deletion of insn with uid = 6.
> > modifying insn i3     8: {r102:SI=r105:SI>>0x4;clobber flags:CC;}
> >        REG_UNUSED flags:CC
> >        REG_DEAD r105:SI
> > deferring rescan insn with uid = 8.
> >
> > This requires two minor tweaks.  The first is that the paradoxical
> > subreg assignment (set (subreg:DI (reg:SI 102) 0) (expr:DI)) can
> > (sometimes) be simplified to (set (reg:SI 102) (truncate:SI (expr:DI))
> > especially if the (truncate:SI (expr:DI)) can itself be simplified.
> > The second is that (truncate:SI (sign_extract:DI (reg:SI 102) x y))
> > slips through the existing simplify_truncation transformations as the
> > the {zero,sign}_extract has a different mode to its first operand.
> >
> > This patch has been tested on x86_64-pc-linux-gnu with make bootstrap
> > and make -k check, both with and without --target_board=unix{-m32},
> > with no new failures.  Ok for mainline?
> >
> > 2026-01-09  Roger Sayle  <roger@nextmovesoftware.com>
> >
> > gcc/ChangeLog
> >          PR rtl-optimization/123236
> >          * combine.cc (simplify_set): Attempt to simplify SETs where the
> >          destination is a low-part paradoxical subreg between scalar integer
> >          modes.
> >          * simplify-rtx.cc (simplify_context::simplify_truncation): Handle
> >          cases where a ZERO_EXTRACT or SIGN_EXTRACT has a different mode
> >          to (at least as wide as) its first operand.
> >
> > gcc/testsuite/ChangeLog
> >          PR rtl-optimization/123236
> >          * gcc.target/i386/pr123236.c: New test case.
> So in simplify_truncation you dropped the REG_P (XEXP (op, 0)) check. Was that
> intentional?  I guess since you don't ultimately change the mode of the truncated
> object that's probably OK.  So as long as it was intentional, I won't object to it.
> 
> So if the removal was intentional, then OK as-is.  If it was a goof, then OK with the
> check restored.

Yes, removing this check was intentional.  As you know, combine smashes together
instructions and relies on simplify-rtx to clean up the resulting "fusion products"
into something reasonable.  So in a machine description, the first operand of a
????_extract is expected to be a register, but during combine it could be anything.
The correctness of this transformation isn’t dependent upon where XEXP (op, 0) is
a register or not, so restricting it just leads to missed optimization opportunities.

I must admit that when removing this test I was thinking about the above RTL:
> > (set (subreg:DI (reg:SI 102 [ _4 ]) 0)
> >      (zero_extend:DI (subreg:SI (sign_extract:DI (reg:SI 105 [ aD.2962 ])
> >                  (const_int 28 [0x1c])
> >                  (const_int 4 [0x4])) 0)))

Here the first operand of a subreg should almost always be a REG (or MEM), but
clearly this is a case where simplify_rtx needs to be able handle something more
exotic.  Indeed a major motivation for this patch is to avoid creating that very
dubious SUBREG in the first place.

To address Richard Sandiford's concerns, I'd hope there a very few places in RISC
backends, where a (middle-end generated) SUBREG of a non-REG/non-MEM rtx
was recognized as a valid instruction.  Having simplify-rtx generate a pattern that
could potentially be recognized (and costed) can only improve code quality.

Along a similar line, I had a related old patch from November 2023,
PR112380: Defend against CLOBBERs in RTX expressions in combine.cc
https://gcc.gnu.org/pipermail/gcc-patches/2023-November/636203.html
that likewise realized that returning a rtx containing (clobber (const_int 0))
is never going to be recognized (should never be recognized), so having
simplify-rtx notice this and try something else, can also only improve code
quality.


I'm happy to wait a little while to investigate usage of paradoxical SUBREG SETs
on aarch64 and other RISC architectures, especially if someone can come up
with a concrete example of things getting worse, rather than just speculation.
The patch can always be reverted if we encounter cases I've not considered,
or there's a proposal to fix this in combine/simplify-rtx somewhere else that
addresses the regression on x86_64.

Cheers,
Roger
--
  
Jeff Law May 5, 2026, 4:40 a.m. UTC | #7
On 5/4/2026 5:32 PM, Roger Sayle wrote:
>>> (set (subreg:DI (reg:SI 102 [ _4 ]) 0)
>>>       (zero_extend:DI (subreg:SI (sign_extract:DI (reg:SI 105 [ aD.2962 ])
>>>                   (const_int 28 [0x1c])
>>>                   (const_int 4 [0x4])) 0)))
> Here the first operand of a subreg should almost always be a REG (or MEM), but
> clearly this is a case where simplify_rtx needs to be able handle something more
> exotic.  Indeed a major motivation for this patch is to avoid creating that very
> dubious SUBREG in the first place.
>
> To address Richard Sandiford's concerns, I'd hope there a very few places in RISC
> backends, where a (middle-end generated) SUBREG of a non-REG/non-MEM rtx
> was recognized as a valid instruction.  Having simplify-rtx generate a pattern that
> could potentially be recognized (and costed) can only improve code quality.
So I looked fairly seriously at using SUBREGs of things like basic 
arithmetic and shifts to express that there are times where we don't 
care about the upper bits, but for "reasons" we can't change the mode.  
   If we do this before combine it opens up more possibilities to use 
the single bit manipulation instructions for RISC-V.  In fact, I suspect 
it's the single largest source of missed bset/bclr/binv opportunities 
for rv64.

I've got prototype code lying around here; I'm not at all happy with it 
right now and it's not in use anywhere.

Philipp and his team are approaching the problem differently by 
promoting the operations to DI when safe.  That may be sufficient as 
well, I need to sit down and play with it for a while.

> I'm happy to wait a little while to investigate usage of paradoxical SUBREG SETs
> on aarch64 and other RISC architectures, especially if someone can come up
> with a concrete example of things getting worse, rather than just speculation.
> The patch can always be reverted if we encounter cases I've not considered,
> or there's a proposal to fix this in combine/simplify-rtx somewhere else that
> addresses the regression on x86_64.
I don't have anything concrete that would be negatively affected by your 
change at this time.


jeff
  
Richard Sandiford May 5, 2026, 8:13 a.m. UTC | #8
[Combining replies because I'm only really making a single point]

"Roger Sayle" <roger@nextmovesoftware.com> writes:
> The same class of bug is also present on aarch64, as an example of a RISC
> target, but there it presents differently in the RTL, due to the internal
> representations making less use of SUBREG.
>
> Consider the four functions:
>
> int sfoo(int a) {
>   long long t = a;
>   return t >> 4;
> }
>
> unsigned int ufoo(unsigned int a) {
>   unsigned long long t = a;
>   return t >> 4;
> }
>
> long long sbar(long long x)
> {
>     __int128 t = x;
>     return t >> 4;
> }
>
> unsigned long long ubar(unsigned long long x)
> {
>     unsigned __int128 t = x;
>     return t >> 4;
> }
>
> With -O2, GCC on aarch64 currently generates:
>
> sfoo:
>         sbfx    x0, x0, 4, 28
>         ret
> ufoo:
>         lsr     w0, w0, 4
>         ret
> sbar:
>         asr     x1, x0, 63
>         extr    x0, x1, x0, 4
>         ret
> ubar:
>         lsr     x0, x0, 4
>         ret
>
> Whereas current clang/LLVM generates (the hoped for):
>
> sfoo:
>         asr     w0, w0, #4
>         ret
>
> ufoo:
>         lsr     w0, w0, #4
>         ret
>
> sbar:
>         asr     x0, x0, #4
>         ret
>
> ubar:
>         lsr     x0, x0, #4
>         ret
>
>
> So the motivation for the patch is independent of the target,
> it's just that some (RISC) backends don’t suffer SUBREG headaches
> to the same extent as others (such as x86 and AVR).

I'm not surprised that you've found an aarch64 case that benefits from
the patch.  To be clear, I wasn't claiming otherwise.  My concerns were:

(1) As far as I can tell, all the motivating examples in the discussion
    have involved cases where applying simplify_gen_unary to the src
    produces a simplification.  But simplify_gen_unary will produce
    a new expression even if no simplification is possible.  Thus if
    we have:

       (set (subreg:DI (reg:SI X) 0) (lshiftrt:DI ....))

    the patch (AFAICT) will produce:

       (set (reg:SI X) (subreg:SI (lshiftrt:DI ...) 0))

    if the shift cannot be simplified.

(2) All the motivating examples have involved cases where the inner
    register of the subreg is a native arithmetic mode for the target.
    But the transformation applies to other modes too, such as QImode
    and HImode on RISCy targets.

"Roger Sayle" <roger@nextmovesoftware.com> writes:
> I must admit that when removing this test I was thinking about the above RTL:
>> > (set (subreg:DI (reg:SI 102 [ _4 ]) 0)
>> >      (zero_extend:DI (subreg:SI (sign_extract:DI (reg:SI 105 [ aD.2962 ])
>> >                  (const_int 28 [0x1c])
>> >                  (const_int 4 [0x4])) 0)))
>
> Here the first operand of a subreg should almost always be a REG (or
> MEM), but clearly this is a case where simplify_rtx needs to be able
> handle something more exotic.  Indeed a major motivation for this
> patch is to avoid creating that very dubious SUBREG in the first
> place.
>
> To address Richard Sandiford's concerns, I'd hope there a very few
> places in RISC backends, where a (middle-end generated) SUBREG of a
> non-REG/non-MEM rtx was recognized as a valid instruction.

Right.  But that's my point :)  If no simplification is possible,
simplify_gen_unary will wrap the original SET_SRC in a subreg or
a truncate (depending on TRULY_NOOP_TRUNCATION).  That isn't likely
to match if the SET_SRC is anything other than a trivial operand.

Even if the simplify_gen_unary does simplify, there are cases where
that might not be what we want.  On a 32-bit WORD_REGISTER_OPERATIONS
target, it seems wrong to try to simplify (say) a 32-bit addition to a
16-bit addition just because the target is (subreg:SI (reg:HI ...) 0).

(FTR, aarch64 is not a WORD_REGISTER_OPERATIONS target, which IMO
is a good thing :)  But many other targets are, of course.)

The crux of my objection is this: it means that, if we have one i3
of the form:

  A: (set (reg:SI RA) src)

and another i3 of the form:

  B: (set (subreg:SI (reg:HI RB) 0) src)

with identical src, and with identical instructions feeding src,
combine will now tend to succeed for A on RISCy targets in ways in
which it wouldn't succeed for B, even though both destinations match
register_operand.  That isn't something that I would have expected
from combine until now.

Like I say, I can see why this transformation is attractive.  It just
feels like something that would be better done at the recog stage.
My original suggestion was to try the transformation if the initial
recog fails, but I suppose that doesn't help with your use case.
An alternative would be to try matching both forms.  If only one
matches, keep that.  If both match, keep the cheapest.  If they're
equally cheap, keep the one with the simple destination.

> Having simplify-rtx generate a pattern that could potentially be
> recognized (and costed) can only improve code quality.
>
> [...]
> I'm happy to wait a little while to investigate usage of paradoxical
> SUBREG SETs on aarch64 and other RISC architectures, especially if
> someone can come up with a concrete example of things getting worse,
> rather than just speculation.  The patch can always be reverted if we
> encounter cases I've not considered, or there's a proposal to fix this
> in combine/simplify-rtx somewhere else that addresses the regression
> on x86_64.

Fair enough.  I'm really just restating my original objection, rather
than adding anything new, so I won't push it further.

Richard
  

Patch

diff --git a/gcc/combine.cc b/gcc/combine.cc
index 66963222efc..b0c930ae5f5 100644
--- a/gcc/combine.cc
+++ b/gcc/combine.cc
@@ -7066,6 +7066,27 @@  simplify_set (rtx x)
       src = SET_SRC (x), dest = SET_DEST (x);
     }
 
+  /* If we have (set (subreg:M (reg:N DEST) SRC) with M wider than N,
+     we have a paradoxical subreg destination, such as created by the
+     clause above, check if we can simplify (subreg:N SRC) to eliminate
+     the (paradoxical) subreg.  */
+  else if (GET_CODE (dest) == SUBREG
+	   && subreg_lowpart_p (dest)
+	   && paradoxical_subreg_p (dest)
+	   && SCALAR_INT_MODE_P (mode)
+	   && SCALAR_INT_MODE_P (GET_MODE (SUBREG_REG (dest))))
+    {
+      rtx tmp = simplify_gen_unary (TRUNCATE, GET_MODE (SUBREG_REG (dest)),
+				    src, mode);
+      if (tmp)
+	{
+	  SUBST (SET_DEST (x), SUBREG_REG (dest));
+	  SUBST (SET_SRC (x), tmp);
+	  dest = SET_DEST (x);
+	  src = SET_SRC (x);
+	}
+    }
+
   /* If we have (set FOO (subreg:M (mem:N BAR) 0)) with M wider than N, this
      would require a paradoxical subreg.  Replace the subreg with a
      zero_extend to avoid the reload that would otherwise be required.
diff --git a/gcc/simplify-rtx.cc b/gcc/simplify-rtx.cc
index 8016e02e925..e27ca8be5eb 100644
--- a/gcc/simplify-rtx.cc
+++ b/gcc/simplify-rtx.cc
@@ -727,12 +727,10 @@  simplify_context::simplify_truncation (machine_mode mode, rtx op,
 	}
     }
 
-  /* Turn (truncate:M1 (*_extract:M2 (reg:M2) (len) (pos))) into
-     (*_extract:M1 (truncate:M1 (reg:M2)) (len) (pos')) if possible without
-     changing len.  */
+  /* Turn (truncate:M1 (*_extract:M2 (reg:M3) (len) (pos))) into
+     (*_extract:M1 (truncate:M1 (reg:M3)) (len) (pos')) if possible.  */
   if ((GET_CODE (op) == ZERO_EXTRACT || GET_CODE (op) == SIGN_EXTRACT)
-      && REG_P (XEXP (op, 0))
-      && GET_MODE (XEXP (op, 0)) == GET_MODE (op)
+      && precision <= GET_MODE_UNIT_PRECISION (GET_MODE (XEXP (op, 0)))
       && CONST_INT_P (XEXP (op, 1))
       && CONST_INT_P (XEXP (op, 2)))
     {
@@ -741,7 +739,8 @@  simplify_context::simplify_truncation (machine_mode mode, rtx op,
       unsigned HOST_WIDE_INT pos = UINTVAL (XEXP (op, 2));
       if (BITS_BIG_ENDIAN && pos >= op_precision - precision)
 	{
-	  op0 = simplify_gen_unary (TRUNCATE, mode, op0, GET_MODE (op0));
+	  if (GET_MODE (op0) != mode)
+	    op0 = simplify_gen_unary (TRUNCATE, mode, op0, GET_MODE (op0));
 	  if (op0)
 	    {
 	      pos -= op_precision - precision;
@@ -751,7 +750,8 @@  simplify_context::simplify_truncation (machine_mode mode, rtx op,
 	}
       else if (!BITS_BIG_ENDIAN && precision >= len + pos)
 	{
-	  op0 = simplify_gen_unary (TRUNCATE, mode, op0, GET_MODE (op0));
+	  if (GET_MODE (op0) != mode)
+	    op0 = simplify_gen_unary (TRUNCATE, mode, op0, GET_MODE (op0));
 	  if (op0)
 	    return simplify_gen_ternary (GET_CODE (op), mode, mode, op0,
 					 XEXP (op, 1), XEXP (op, 2));
diff --git a/gcc/testsuite/gcc.target/i386/pr123236.c b/gcc/testsuite/gcc.target/i386/pr123236.c
new file mode 100644
index 00000000000..b369b6fd299
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr123236.c
@@ -0,0 +1,18 @@ 
+/* { dg-do compile { target { ! ia32 } } } */
+/* { dg-options "-O2" } */
+
+unsigned int foo(unsigned int a) {
+  unsigned long long t = a;
+  return t >> 4;
+}
+
+int bar(int a) {
+  long long t = a;
+  return t >> 4;
+}
+
+/* { dg-final { scan-assembler-not "movq" } } */
+/* { dg-final { scan-assembler-not "salq" } } */
+/* { dg-final { scan-assembler-not "shrq" } } */
+/* { dg-final { scan-assembler-not "movslq" } } */
+/* { dg-final { scan-assembler-not "sarq" } } */