> 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.
Regardless of the context, ABI conventions, or any other consideration,
a move (set (reg:SI) (reg:SI)) should _not_ require mov32, because the
source register is supposed to already have the upper bits zeroed, or it
would not contain a value of mode SI.
The different movement/extension/etc insns conform a sort of a closure:
all the possible ways to put a SI value into a register, be it from
register or memory or immediate, must result into properly handled upper
32/48/56 bits. Moves from registers and immediates with values of
different moves involve expansions of extend* and friends, moves from
memory involves ldx and friends, all of which handle upper unused bits
properly. Given this, moves from registers of _the same_ mode should
not require anything extra than just copying all the 64 bits, i.e. mov.
So I think that if you can contrive an example in which you would need
emitting mov32 for such a move then the problem is somewhere else, and
you probably should ask yourself, why are the source register's contents
invalid.
You probably want to avoid optimizing while investigating this, because
otherwise a truncation + move will very likely be conflated into a
single perhaps-truncating-move and you will not be able to determine
what the source of the problem is..
