[PATCH 2/3] bpf: md: fix "*movsi" to generate wN regs [PR124688]

[Vineet Gupta]

On 3/31/26 3:42 PM, David Faust wrote:
>>> So then the only pattern that needs to change here would be
>>> the register-register 'mov' insn itself. But the pattern is
>>> matching a move from MM to MM, i.e. the source and dest must
>>> already be the same mode. So are you sure we would need to emit
>>> w reg in this case?
>> Right, the modes not matching is not the issue, it emitting rN reg when
>> both operands are SImode is the problem which I describe in changelog above.
> Hm...
>
>> this can be a correctness issue where mov needs to zero out the upper bits.
> I guess I am missing something but honestly I am confused by this part.
> Why would mov need to zero out the upper bits?
>
> I mean, this case is matching only say a (set (reg:SI rD) (reg:SI rS)).
> i.e. the source register already has an SImode value.  We aren't counting
> on the state of the bits outside that SImode value.  Moving either the
> low 32-bits or all 64, doesn't make a difference to that SImode value.

I think I'm starting to see where you are coming from.


> Explicitly zeroing the upper 32 bits, while it may be achieved by using
> a _BPF_ mov32 insn, I don't think is an exact match to the semantics of
> a simple reg-to-reg move.

Seems fair.

> I understood from the testcase/pr that you are saying the issue is with
> function return/argument passing and extension.  But I don't think this
> simple case matching on set reg:MM reg:MM is the proper way to
> address that.

Right and that's not the intent, this is not claiming to fix the ABI, 
that's a totally different change, this is just the precursor, at least 
in my mind's eye, but I'm willing to concede that view-point.

So let me step back, all of this originated from ABI change work and 
roughly based on HJ's v1 (not v2), I was starting to see the light. 
There was a grand total of 1 additional selftest failure which was 
attach_probe. The manually reduced test (attached here) showed the 
failing case (is essentially beefier version of the test in this patch, 
but same in spirit)

So with the ABI hooks and everything in place, what we were seeing was 
following RTL stream

(call_insn 7 6 8 2 (set (reg:SI 0 %r0)
         (call (mem:DI (symbol_ref:DI ("bpf_copy_from_user_str") [flags 
0x41]  <function_decl 0x7f10c1547300 bpf_copy_from_user_str>) [0 
bpf_copy_from_user_str S8 A64])
             (const_int 0 [0])))
      (expr_list:REG_CALL_DECL (symbol_ref:DI ("bpf_copy_from_user_str") 
[flags 0x41]  <function_decl 0x7f10c1547300 bpf_copy_from_user_str>)
         (nil))
     (expr_list:SI (use (reg:SI 1 %r1))
         (nil)))

(insn 8 7 9 2 (set (reg/v:SI 19 [ ret ])
         (reg:SI 0 %r0))
      (nil))

(jump_insn 9 8 10 2 (set (pc)
       (if_then_else (ne (reg/v:SI 19 [ ret ])
                 (const_int 4 [0x4]))
             (label_ref:DI 33)
             (pc))) {*branch_on_si}
  -> 33)

(insn 11 10 12 4 (set (reg:SI 1 %r1)
         (const_int 4 [0x4]))
      (nil))

cse1 equivalence logic deduces that reg 19 = 4 = SI, guaranteed by 
control flow, so we end up with

(insn 11 10 12 3 (set (reg:SI 1 %r1)
        (reg/v:SI 19 [ ret ])) {*movsi}
      (expr_list:REG_EQUAL (const_int 4 [0x4])
         (nil)))

And this carries all the way to the end, which if generates rN is would 
be problematic and was the reason for this change.

>>> Maybe I am wrong but to me it seems the behavior in the test
>>> below (and in the pr) must be more related to extendhisi.
>> Yes and No :-)
>>
>> You are right that for the test below, it starts of with extend, sidi2
>> to be precise, which gets expanded to 2 shifts (Its on my todo to avoid
>> doing this for cpuv4 for better combine outcomes, but anyways that's for
>> later).
>>
>> (insn 8 7 9 2 (set (reg:DI 19 [ _1 ])
>>           (ashift:DI (subreg:DI (reg:SI 21) 0)
>>               (const_int 32 [0x20])))
>> (insn 9 8 10 2 (set (reg:DI 19 [ _1 ])
>>           (ashiftrt:DI (reg:DI 19 [ _1 ])
>>               (const_int 32 [0x20])))
>> (insn 10 9 11 2 (set (reg:SI 1 %r1)
>>           (subreg/s/u:SI (reg:DI 19 [ _1 ]) 0))
>>
>> Combine is able to see thru all this to generate insn 14
> OK, the value is sign extended to DImode, and then truncated
> back to SImode.  The original value in SImode is unchanged...
>
>> scanning new insn with uid = 14.
>> ...
>> allowing combination of insns 8 and 9
>> deferring deletion of insn with uid = 8.
>> allowing combination of insns 9 and 10
>> deferring deletion of insn with uid = 9.
>> rescanning insn with uid = 10.
>>
>> (insn 14 5 6 2 (set (reg:SI 23)
>>           (reg:SI 0 %r0))  {*movsi}
> ... so this result from combine is valid; the net effect
> is simply to move the SImode value from one reg to another,
> keeping SImode.
>
> I'm not sure exactly what is supposed to happen in the test.
> The arg is passed as int, i.e. SImode. The origin of that value
> is another int, also SImode. Why do the upper bits of the reg need
> to be explicitly zeroed?

You may be right that test case is more ABI leaning (and this pinskia 
said something on PR), but even with the ABI change, this adjustment 
will still be needed IMO.

Thx,
-Vineet
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ret-reuse-arg-2.c
Type: text/x-csrc
Size: 893 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/bpf/attachments/20260331/b22efafb/attachment.bin>