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
/* 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 } } */
