On 3/30/26 13:57, Vineet Gupta wrote:
> Hi David,
>
> Thanks for quickly reviewing the patches.
>
> On 3/30/26 11:45 AM, David Faust wrote:
>>> From: Vineet Gupta <[email protected]>
>
> This somehow snuck in, will fix in v2.
>
>>> movsi currently only generates DImode rN regs, despite RTL being SImode.
>>>
>>> | (insn 14 5 11 (set (reg:SI 1 %r1 [23])
>>> | (reg:SI 0 %r0)) {*movsi}
>>> | (expr_list:REG_DEAD (reg:SI 0 %r0)
>>> | (nil)))
>>>
>>> generates
>>>
>>> | r1 = r0
>>>
>>> as opposed to
>>>
>>> | w1 = w0
>
> [snip...]
>
>>> - *return bpf_output_move (operands, \"{ldx<mop>\t%0,%1|%0 = *(<smop> *)
>>> %1}\");
>>> - *return bpf_output_move (operands, \"{mov\t%0,%1|%0 = %1}\");
>>> - *return bpf_output_move (operands, \"{lddw\t%0,%1|%0 = %1 ll}\");
>>> - *return bpf_output_move (operands, \"{stx<mop>\t%0,%1|*(<smop> *) %0 =
>>> %1}\");
>>> - *return bpf_output_move (operands, \"{st<mop>\t%0,%1|*(<smop> *) %0 =
>>> %1}\");"
>>> + *return bpf_output_move (operands, \"{ldx<mop>\t%0,%1|%w0 = *(<smop> *)
>>> %w1}\");
>>> + *return bpf_output_move (operands, \"{mov\t%0,%1|%w0 = %w1}\");
>>> + *return bpf_output_move (operands, \"{lddw\t%0,%1|%w0 = %w1 ll}\");
>>> + *return bpf_output_move (operands, \"{stx<mop>\t%0,%1|*(<smop> *) %w0 =
>>> %w1}\");
>>> + *return bpf_output_move (operands, \"{st<mop>\t%0,%1|*(<smop> *) %w0 =
>>> %w1}\");"
>> Based on our discussion in the BPF GCC meeting this morning
>> I think we decided that we should not emit wN for ld/st[x],
>> (though gas should accommodate them).
>
> Correct, will do that in v2.
>
>> And the lddw pattern doesn't need to accommodate w regs since
>> it is exclusively for immediate values.
>
> Correct, will revert that as well.
>
>> 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.
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.
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.
>
>> 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?
>
> And the -dP annotated output also confirms movsi involvement for the
> actual asm.
>
> #(insn 14 5 11 (set (reg:SI 1 %r1 [23])
> # (reg:SI 0 %r0))
> "gcc/testsuite/gcc.target/bpf/ret-reuse-arg-1.c":10:4 35 {*movsi}
> # (expr_list:REG_DEAD (reg:SI 0 %r0)
> # (nil)))
> r1 = r0 # 14 [c=4 l=8] *movsi/1
>
>
> Spurred by your comment, I added a variant with "short" ret/args and
> looks like movhi needs even more fixing as it needs to generate wN =
> (s16) wM. I need to see if that needs to fixed in asm pattern or will
> PROMOTE_MODE change later be able to handle that in the core.
>
> Thx,
> -Vineet
