Merge pull request #408 from Wuerfel21/W21-better-strf-fix

Even better fix for SafeToReplaceForward bug

Test/Expect/stest303.pasm

@@ -0,0 +1,72 @@
+pub main
+  coginit(0, @entry, 0)
+dat
+	org	0
+entry
+
+_uninlinable
+_uninlinable_ret
+	ret
+
+_tryit2
+	mov	arg02, arg01 wz
+	mov	tryit2_tmp001_, #99
+	wrlong	tryit2_tmp001_, objptr
+ if_e	mov	_tryit2_varname_S, ptr_L__0002_
+ if_e	mov	tryit2_tmp001_, #2
+ if_e	wrlong	tryit2_tmp001_, objptr
+	cmp	arg02, #1 wz
+ if_e	mov	_tryit2_varname_S, ptr_L__0004_
+ if_e	mov	tryit2_tmp001_, #3
+ if_e	wrlong	tryit2_tmp001_, objptr
+	rdlong	arg01, objptr
+	mov	arg02, ptr_L__0005_
+	call	#_uninlinable
+	rdlong	arg01, objptr
+	mov	arg02, _tryit2_varname_S
+	call	#_uninlinable
+_tryit2_ret
+	ret
+
+_program
+	mov	arg01, #0
+	call	#_tryit2
+	mov	arg01, #1
+	call	#_tryit2
+	mov	arg01, #2
+	call	#_tryit2
+_program_ret
+	ret
+
+objptr
+	long	@@@objmem
+ptr_L__0002_
+	long	@@@LR__0001
+ptr_L__0004_
+	long	@@@LR__0002
+ptr_L__0005_
+	long	@@@LR__0003
+COG_BSS_START
+	fit	496
+
+LR__0001
+	byte	"part0"
+	byte	0
+LR__0002
+	byte	"part1"
+	byte	0
+LR__0003
+	byte	"aha"
+	byte	0
+objmem
+	long	0[1]
+	org	COG_BSS_START
+_tryit2_varname_S
+	res	1
+arg01
+	res	1
+arg02
+	res	1
+tryit2_tmp001_
+	res	1
+	fit	496

Test/stest303.bas

@@ -0,0 +1,25 @@
+
+dim ct as integer
+
+sub for "noinline" uninlinable(lmao as ulong,str as string)
+end sub
+
+sub tryit2(linetype as ulong)
+dim varname$ as string
+dim suffix$ as string
+ct=99
+if linetype=0 then
+  varname$="part0" : ct = 2
+end if
+if linetype=1 then
+  varname$="part1" : ct = 3
+end if
+'print "called tryit with linetype",linetype
+suffix$=varname$ ' right$(varname$,1)
+uninlinable(ct,"aha")
+uninlinable(ct,suffix$)
+end sub
+
+tryit2(0)
+tryit2(1)
+tryit2(2)

backends/asm/optimize_ir.c

@@ -1409,6 +1409,7 @@ SafeToReplaceForward(IR *first_ir, Operand *orig, Operand *replace, IRCond sette
     bool assignments_are_safe = true;
     bool orig_modified = false;
     bool isCond = (setterCond != COND_TRUE);
+    bool condition_safe = true;
 
     if (SrcOnlyHwReg(replace) || !IsRegister(replace->kind)) {
         return NULL;
@@ -1519,7 +1520,7 @@ SafeToReplaceForward(IR *first_ir, Operand *orig, Operand *replace, IRCond sette
                 return NULL;
             }
         }
-        if (!CondIsSubset(setterCond,ir->cond) && InstrUses(ir,orig)) {
+        if ((!condition_safe || !CondIsSubset(setterCond,ir->cond)) && InstrUses(ir,orig)) {
             return NULL;
         }
         if (InstrModifies(ir,replace)) {
@@ -1531,7 +1532,7 @@ SafeToReplaceForward(IR *first_ir, Operand *orig, Operand *replace, IRCond sette
             //      value is being put into it
             //  if "assignments_are_safe" is false then we don't know if another
             //  branch might still use "replace", so punt and give up
-            if (!CondIsSubset(ir->cond,setterCond)) {
+            if (ir->cond != COND_TRUE && (!condition_safe || !CondIsSubset(ir->cond,setterCond))) {
                 return NULL;
             }
             if (ir->dst->kind == REG_SUBREG || replace->kind == REG_SUBREG) {
@@ -1562,7 +1563,10 @@ SafeToReplaceForward(IR *first_ir, Operand *orig, Operand *replace, IRCond sette
                 // sub registers are complicated, punt
                 return NULL;
             }
-            if (ir->cond != setterCond) {
+            if (!condition_safe) {
+                // Can't prove that ir->cond is equal / a subset anymore, thus skip below branch.
+                return NULL;
+            } else if (ir->cond != setterCond) {
                 assignments_are_safe = false;
                 if (!CondIsSubset(setterCond,ir->cond)) {
                     // Not a subset of the setter condition, can't replace
@@ -1572,7 +1576,7 @@ SafeToReplaceForward(IR *first_ir, Operand *orig, Operand *replace, IRCond sette
             if (!InstrUses(ir, orig) && assignments_are_safe) {
                 // we are completely re-setting "orig" here, so we can just
                 // leave now
-                return (last_ir && IsDeadAfter(last_ir, orig)) ? last_ir : NULL;
+                return last_ir;
             }
             // we do not want to end accidentally modifying "replace" if it is still live
             // note that IsDeadAfter(first_ir, replace) gives a more accurate
@@ -1584,12 +1588,15 @@ SafeToReplaceForward(IR *first_ir, Operand *orig, Operand *replace, IRCond sette
             }
             orig_modified = true;
         }
+        if (isCond && InstrSetsFlags(ir,FlagsUsedByCond(setterCond))) {
+            // Setters condition is no longer valid.
+            // Technically, condition is still safe if it's logically AND-ed, i.e "if_z test x,y wz",
+            // but we're not going there now.
+            condition_safe = false;
+        }
         last_ir = ir;
     }
-    if (last_ir && IsLocalOrArg(orig) && IsDeadAfter(last_ir, orig)) {
-        return last_ir;
-    }
-    return NULL;
+    return IsLocalOrArg(orig) ? last_ir : NULL;
 }