On 3/31/26 16:22, Vineet Gupta wrote:
>
> 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)
Pasting here the attached test for reference:
> /* Extracted from BPF selftests attach_probe: verifier failure.
> Return value of first call is reused as argument of second.
> It either needs to be explicitly truncated to s32 or w regs need
> to be used. */
> /* { dg-do compile } */
> /* { dg-options "-O2 -mcpu=v4" } */
>
> extern int bpf_copy_from_user_str(int dst__sz);
>
> _Bool verify_sleepable_user_copy_str(void)
> {
> int ret;
> char data_short_pad[4];
>
> ret = bpf_copy_from_user_str(sizeof(data_short_pad));
> if (ret != 4)
> return false;
>
> /* ret above is guaranteed to be 4, same as sizeof. */
> ret = bpf_copy_from_user_str(sizeof(data_short_pad));
> if (ret != 4)
> return false;
>
> return true;
> }
>
> /* Don't generate 64-bit mov for arg, do 32-bit zero extended mov. */
> /* { dg-final { scan-assembler-not {r1 = r0} } } */
> /* { dg-final { scan-assembler-not {r1 = \(s32\) r0} } } */
> /* { dg-final { scan-assembler-times {w1 = w0} 1 } } */
I'm confused by the comment at the top.
First:
> Return value of first call is reused as argument of second.
Where? The only calls I see are identical:
ret = bpf_copy_from_user_str (sizeof (data_short_pad));
The return value is not reused as an argument.
Secondly:
> It either needs to be explicitly truncated to s32 or w regs need
> to be used. */
Why? Assuming the value were actually being reused, we would be
taking an int return value and passing it as an int argument to a
function taking an int. There is no conversion needed.
>
> So with the ABI hooks and everything in place, what we were seeing was
> following RTL stream
In the RTL below it's clear that the values are all being treated
in SImode:
>
> (call_insn 7 6 8 2 (set (reg:SI 0 %r0)
So the call return value is SImode.
> (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))
(use (reg:SI 1 %r1)) : the arg is passed SI mode
> (nil)))
>
> (insn 8 7 9 2 (set (reg/v:SI 19 [ ret ])
> (reg:SI 0 %r0))
Return value copied to pseudo r19 in SImode to be used for
the comparison.
> (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
Note that r19 is SImode a priori, to match the mode of the value from %r0.
It is not SI mode as a result of cse1 determining that it holds 4.
>
> (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.
And again, what precisely is problematic about generating rN = rM
for this set?
If %r1 is subsequently used as the int argument to a function call,
then it is valid as is.
If it would be passed as a 'unsigned long' or something else requiring
a conversion then we would rather have e.g. a zero_extend:DI to do that,
not a simple set.
>
>>>> 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
