query about commit 666fdc46bc8489 ("RISC-V: Fix bad insn splits with paradoxical subregs")

query about commit 666fdc46bc8489 ("RISC-V: Fix bad insn splits with paradoxical subregs")

[Vineet Gupta]

Hi Jakub,

I had a question about the aforementioned commit in RV backend.

(define_split
   [(set (match_operand:GPR 0 "register_operand")
	(and:GPR (match_operand:GPR 1 "register_operand")
                (match_operand:GPR 2 "p2m1_shift_operand")))
+   (clobber (match_operand:GPR 3 "register_operand"))]
   ""
-  [(set (match_dup 0)
+  [(set (match_dup 3)
        (ashift:GPR (match_dup 1) (match_dup 2)))

Is there something specific to this split which warrants this or so any 
split patterns involving shifts have this to avoid the shifting by more 
than SUBREG_REG problem.

Also could you please explain where the clobber itself is allocated ?

This came up when discussing the solution to PR/106602 [1]


Thx,
-Vineet

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106602

Re: query about commit 666fdc46bc8489 ("RISC-V: Fix bad insn splits with paradoxical subregs")

[Jeff Law]

On 11/4/22 16:59, Vineet Gupta wrote:
> Hi Jakub,
>
> I had a question about the aforementioned commit in RV backend.
>
> (define_split
>   [(set (match_operand:GPR 0 "register_operand")
>     (and:GPR (match_operand:GPR 1 "register_operand")
>                (match_operand:GPR 2 "p2m1_shift_operand")))
> +   (clobber (match_operand:GPR 3 "register_operand"))]
>   ""
> -  [(set (match_dup 0)
> +  [(set (match_dup 3)
>        (ashift:GPR (match_dup 1) (match_dup 2)))
>
> Is there something specific to this split which warrants this or so 
> any split patterns involving shifts have this to avoid the shifting by 
> more than SUBREG_REG problem.

Not sure.  Note it was Jim Wilson's change, not Jakub's change AFAICT:


commit 666fdc46bc848984ee7d2906f2dfe10e1ee5d535
Author: Jim Wilson <jimw@sifive.com>
Date:   Sat Jun 30 21:52:01 2018 +0000

     RISC-V: Add patterns to convert AND mask to two shifts.

             gcc/
             * config/riscv/predicates.md (p2m1_shift_operand): New.
             (high_mask_shift_operand): New.
             * config/riscv/riscv.md (lshrsi3_zero_extend_3+1): New combiner
             pattern using p2m1_shift_operand.
             (lshsi3_zero_extend_3+2): New combiner pattern using
             high_mask_shift_operand.

             gcc/testsuite/
             * gcc.target/riscv/shift-shift-1.c: New.
             * gcc.target/riscv/shift-shift-2.c: New.
             * gcc.target/riscv/shift-shift-3.c: New.

     From-SVN: r262278

You might find further discussion in the gcc-patches archives.


>
> Also could you please explain where the clobber itself is allocated ?

I'd expect the combiner.   There's going to be 2 or more insns that are 
combined to create this pattern where the output of one feeds a 
subsequent insn and dies.  So it's effectively a temporary.  Combine 
will re-use that temporary as the clobbered operand.


Jeff

Re: query about commit 666fdc46bc8489 ("RISC-V: Fix bad insn splits with paradoxical subregs")

[Vineet Gupta]

On 11/4/22 16:13, Jeff Law wrote:
>
> On 11/4/22 16:59, Vineet Gupta wrote:
>> Hi Jakub,
>>
>> I had a question about the aforementioned commit in RV backend.
>>
>> (define_split
>>   [(set (match_operand:GPR 0 "register_operand")
>>     (and:GPR (match_operand:GPR 1 "register_operand")
>>                (match_operand:GPR 2 "p2m1_shift_operand")))
>> +   (clobber (match_operand:GPR 3 "register_operand"))]
>>   ""
>> -  [(set (match_dup 0)
>> +  [(set (match_dup 3)
>>        (ashift:GPR (match_dup 1) (match_dup 2)))
>>
>> Is there something specific to this split which warrants this or so 
>> any split patterns involving shifts have this to avoid the shifting 
>> by more than SUBREG_REG problem.
>
> Not sure.  Note it was Jim Wilson's change, not Jakub's change AFAICT:
>
>
> commit 666fdc46bc848984ee7d2906f2dfe10e1ee5d535
> Author: Jim Wilson <jimw@sifive.com>
> Date:   Sat Jun 30 21:52:01 2018 +0000
>
>     RISC-V: Add patterns to convert AND mask to two shifts.
>
>             gcc/
>             * config/riscv/predicates.md (p2m1_shift_operand): New.
>             (high_mask_shift_operand): New.

Indeed Jim introduced the pattern with 666fdc46bc8, but the clobber was 
added later in 36ec3f57d305 ("RISC-V: Fix bad insn splits with 
paradoxical subregs"). He attributed this to Jakub, and with Jim not 
being super active these days, I tried reaching out to this cc list.

Sorry I pasted wrong sha-id in my orig msg, it needs to be 36ec3f57d305


>
> You might find further discussion in the gcc-patches archives.

I'll dig some more.

>
>>
>> Also could you please explain where the clobber itself is allocated ?
>
> I'd expect the combiner.   There's going to be 2 or more insns that 
> are combined to create this pattern where the output of one feeds a 
> subsequent insn and dies.  So it's effectively a temporary.  Combine 
> will re-use that temporary as the clobbered operand.

Make sense.

Thx,
-Vineet

Re: query about commit 666fdc46bc8489 ("RISC-V: Fix bad insn splits with paradoxical subregs")

[Jeff Law]

On 11/4/22 17:38, Vineet Gupta wrote:
>> commit 666fdc46bc848984ee7d2906f2dfe10e1ee5d535
>> Author: Jim Wilson <jimw@sifive.com>
>> Date:   Sat Jun 30 21:52:01 2018 +0000
>>
>>     RISC-V: Add patterns to convert AND mask to two shifts.
>>
>>             gcc/
>>             * config/riscv/predicates.md (p2m1_shift_operand): New.
>>             (high_mask_shift_operand): New.
>
> Indeed Jim introduced the pattern with 666fdc46bc8, but the clobber 
> was added later in 36ec3f57d305 ("RISC-V: Fix bad insn splits with 
> paradoxical subregs"). He attributed this to Jakub, and with Jim not 
> being super active these days, I tried reaching out to this cc list.
>
> Sorry I pasted wrong sha-id in my orig msg, it needs to be 36ec3f57d305

I'd look at the testcases in that hash.  I bet you're going to find one 
where we have a paradoxical subreg destination.  We can't shift bits 
into the paradoxical part, then expect to safely shift them back in.  
The semantics of a paradoxical subreg are that the bits outside the 
inner mode are don't cares.  Of course you *may* need to go back to a 
compiler a that point in time to see the problematical case.


Jeff