Message-Id: <C823738D-8FE1-4746-A8CF-627DFB596365@jrtc27.com>
Date: Fri, 14 Feb 2025 06:04:53 +0000
From: Jessica Clarke <jrtc27@...c27.com>
To: yunhui cui <cuiyunhui@...edance.com>
Cc: patchwork-bot+linux-riscv@...nel.org,
 Andreas Schwab <schwab@...e.de>,
 linux-riscv@...ts.infradead.org,
 linux-kernel@...r.kernel.org
Subject: Re: [External] [PATCH] riscv/futex: sign extend compare value in
 atomic cmpxchg

On 14 Feb 2025, at 04:11, yunhui cui <cuiyunhui@...edance.com> wrote:
> 
> Hi,
> 
> On Fri, Feb 14, 2025 at 2:31 AM <patchwork-bot+linux-riscv@...nel.org> wrote:
>> 
>> Hello:
>> 
>> This patch was applied to riscv/linux.git (fixes)
>> by Palmer Dabbelt <palmer@...osinc.com>:
>> 
>> On Mon, 03 Feb 2025 11:06:00 +0100 you wrote:
>>> Make sure the compare value in the lr/sc loop is sign extended to match
>>> what lr.w does.  Fortunately, due to the compiler keeping the register
>>> contents sign extended anyway the lack of the explicit extension didn't
>>> result in wrong code so far, but this cannot be relied upon.
>>> 
>>> Fixes: b90edb33010b ("RISC-V: Add futex support.")
>>> Signed-off-by: Andreas Schwab <schwab@...e.de>
>>> 
>>> [...]
>> 
>> Here is the summary with links:
>>  - riscv/futex: sign extend compare value in atomic cmpxchg
>>    https://git.kernel.org/riscv/c/5c238584bce5
>> 
>> You are awesome, thank you!
>> --
>> Deet-doot-dot, I am a bot.
>> https://korg.docs.kernel.org/patchwork/pwbot.html
>> 
>> 
> 
> I applied this patch, but it doesn't seem to take effect. There is no
> sign extension for a2 in the assembly code. What did I miss?
> GCC version >= 13.
> 
> ffffffff800e87e0 <futex_atomic_cmpxchg_inatomic>:
> ffffffff800e87e0: 1141                addi sp,sp,-16
> ffffffff800e87e2: e422                sd s0,8(sp)
> ffffffff800e87e4: 2bf01793          bseti a5,zero,0x3f
> ffffffff800e87e8: 0800                addi s0,sp,16
> ffffffff800e87ea: 17ed                addi a5,a5,-5
> ffffffff800e87ec: 00b7f663          bgeu a5,a1,ffffffff800e87f8
> <futex_atomic_cmpxchg_inatomic+0x18>
> ffffffff800e87f0: 6422                ld s0,8(sp)
> ffffffff800e87f2: 5549                li a0,-14
> ffffffff800e87f4: 0141                addi sp,sp,16
> ffffffff800e87f6: 8082                ret
> ffffffff800e87f8: 872a                mv a4,a0
> ffffffff800e87fa: 00040837          lui a6,0x40
> ffffffff800e87fe: 10082073          csrs sstatus,a6
> ffffffff800e8802: 4781                li a5,0
> ffffffff800e8804: 1605a8af          lr.w.aqrl a7,(a1)
> ffffffff800e8808: 00c89563          bne a7,a2,ffffffff800e8812
> <futex_atomic_cmpxchg_inatomic+0x32>
> ffffffff800e880c: 1ed5a52f          sc.w.aqrl a0,a3,(a1)
> ffffffff800e8810: f975                bnez a0,ffffffff800e8804
> <futex_atomic_cmpxchg_inatomic+0x24>
> ffffffff800e8812: 0007851b          sext.w a0,a5
> ffffffff800e8816: 10083073          csrc sstatus,a6
> ffffffff800e881a: 01172023          sw a7,0(a4)
> ffffffff800e881e: 6422                ld s0,8(sp)
> ffffffff800e8820: 0141                addi sp,sp,16
> ffffffff800e8822: 8082                ret

The calling convention means a2 will be sign-extended on entry to the
function, and since your compiler has not done anything to change the
value that will still be true. So it has (legitimately) optimised out
any sign extension as a no-op. Are you seeing any problems that you
believe to be caused by this function, or are you just inspecting the
disassembly to satisfy your own curiosity?

> Should we do it like this:
> __asm__ __volatile__ (
> " sext.w %[ov], %[ov] \n"
> ...

No, it’s unnecessary and prevents optimisation.

Jess

> Thanks,
> Yunhui
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Powered by blists - more mailing lists