Viper destructive unary integer operations clobber register-backed local variables

[samneggs]

Could someone check this?

@micropython.viper
def test():
    n = 1
    n1 = -n
    print(n) # should print 1

Also for ~n

Sam

[GitHubsSilverBullet]

Error verified on RP2040 and RP2350 (both v1.28.0)
Somehow I get the impression as if the viper stuff was much more stable a couple of releases ago.

[bixb922]

Error verified on ESP32-S3 with v1.28.0,

This is most probably related to #14397 Viper: Unary minus changes rhs variable in place, even if assigned to new lhs variable.

The viper wiki: https://github.com/micropython/micropython/wiki/Improving-performance-with-Viper-code also states:

Be aware: In versions MicroPython 1.22 and prior, unary minus is not implemented, instead of x=-a use x=0-a. In version 1.23 the unary minus is being implemented, but not completely yet, so until further confirmation it's best to avoid unary minus, and use subtraction from zero instead."

No mention of unary invert in the wiki.

(Edited)

[samneggs]

Yep, that's it from April 2024.
A compiled error is better than a runtime bug but I don't want to be ungrateful.
Sam

[agatti]

This is a tough one.

If I remember correctly back when I looked at it (more than a year ago!), by the time the assembly instructions were scheduled for generation the reference to the old value was already thrown away by the compiler. To elaborate further, it'd pre-optimise the statement by negating the second variable first and then passing that for assignment. It assumed the same internal variable semantics that apply to regular Python bytecode also apply to emitted code, which is sadly not the case.

I can have a fresher look at it tomorrow, but I fear the outcome is the same. Either that bit of the compiler gets rewritten for both bytecode and emitted opcodes (trying to keep the footprint impact to a minimum too), or there has to be some other solution I did not see at the time.

[agatti]

Oh well, getting away from something and then back to it seems to help. I guess I was fixated on how to solve it on the compiler side rather than on the emitter side. This seems to fix it, but I have the feeling it breaks plenty of other stuff - even though running tests via --via-mpy --emit=native passes on x64 (and on Arm, RV32, and Xtensa too)!

@samneggs Can you please try with this patch?

diff --git i/py/emitnative.c w/py/emitnative.c
index 7533f8374..29276c66d 100644
--- i/py/emitnative.c
+++ w/py/emitnative.c
@@ -2338,22 +2338,18 @@ static void emit_native_unary_op(emit_t *emit, mp_unary_op_t op) {
         if (op == MP_UNARY_OP_POSITIVE) {
             // No-operation, just leave the argument on the stack.
         } else if (op == MP_UNARY_OP_NEGATIVE) {
-            int reg = REG_RET;
-            emit_pre_pop_reg_flexible(emit, &vtype, &reg, reg, reg);
-            ASM_NEG_REG(emit->as, reg);
-            emit_post_push_reg(emit, vtype, reg);
+            emit_pre_pop_reg(emit, &vtype, REG_RET);
+            ASM_NEG_REG(emit->as, REG_RET);
+            emit_post_push_reg(emit, vtype, REG_RET);
         } else if (op == MP_UNARY_OP_INVERT) {
+            emit_pre_pop_reg(emit, &vtype, REG_RET);
             #ifdef ASM_NOT_REG
-            int reg = REG_RET;
-            emit_pre_pop_reg_flexible(emit, &vtype, &reg, reg, reg);
-            ASM_NOT_REG(emit->as, reg);
+            ASM_NOT_REG(emit->as, REG_RET);
             #else
-            int reg = REG_RET;
-            emit_pre_pop_reg_flexible(emit, &vtype, &reg, REG_ARG_1, reg);
             ASM_MOV_REG_IMM(emit->as, REG_ARG_1, -1);
-            ASM_XOR_REG_REG(emit->as, reg, REG_ARG_1);
+            ASM_XOR_REG_REG(emit->as, REG_RET, REG_ARG_1);
             #endif
-            emit_post_push_reg(emit, vtype, reg);
+            emit_post_push_reg(emit, vtype, REG_RET);
         } else {
             EMIT_NATIVE_VIPER_TYPE_ERROR(emit,
                 MP_ERROR_TEXT("'not' not implemented"), mp_binary_op_method_name[op]);