I've noticed cproc generating what I'm fairly sure are redundant QBE
cnew instructions.

Simple test case:

int
test(int v)
{
    if (v == 45 || v == 47)
        return 1;
    return 0;
}

Generates:

export
function w $test(w %.1) {
@start.1
    %.2 =l alloc4 4
    storew %.1, %.2
@body.2
    %.3 =w loadw %.2
    %.4 =w ceqw %.3, 45
    jnz %.4, @logic_join.4, @logic_right.3
@logic_right.3
    %.5 =w loadw %.2
    %.6 =w ceqw %.5, 47
    %.7 =w cnew %.6, 0     <------ THIS !!!
@logic_join.4
    %.8 =w phi @body.2 1, @logic_right.3 %.7
    jnz %.8, @if_true.5, @if_false.6
@if_true.5
    ret 1
@if_false.6
    ret 0
}

I'm fairly sure the cnew marked above is a nop (well copy to be precise).

I've added QBE code to deal with this in (GVN) copy detection, but
interested in why?

Thanks, Roland

FWIW the QBE (GVN) patch is as follows, on top of (GVN/GCM) patches in
https://lists.sr.ht/~mpu/qbe/%3CCAAS8gYBwVe0qcTnEOUuQLH4OVE2XuUF4V8MRHYZTb86U11+e-A@mail.gmail.com%3E.

+++ b/gvn.c
@@ -336,6 +336,7 @@ copyarg(Fn *fn, Ins *i)
        };
        bits b;
        Tmp *t;
+       int argn, x;

        if (i->op == Ocopy)
                return i->arg[0];
@@ -358,6 +359,19 @@ copyarg(Fn *fn, Ins *i)
                // TODO - check - assumes Cons, Syms are deduped
                if (rtype(i->arg[0]) == RCon && rtype(i->arg[1]) == RCon)
                        return con01[1-optab[i->op].eqval];
+               if (i->op == Ocnew || i->op == Oceqw) {
+                       argn = -1;
+                       if (isconref(fn, i->arg[0], i->cls, optab[i->op].eqval))
+                               argn = 1;
+                       else if (isconref(fn, i->arg[1], i->cls,
optab[i->op].eqval))
+                               argn = 0;
+                       if (argn != -1 && rtype(i->arg[argn]) == RTmp) {
+                               t = &fn->tmp[i->arg[argn].val];
+                               if (t->def)
+                               if (iscmp(t->def->op, &x, &x))
+                                       return i->arg[argn];
+                       }
+               }
        }

        if (!isext(i->op) || rtype(i->arg[0]) != RTmp)

Hi Roland,

Roland Paterson-Jones <rolandpj@gmail.com> wrote:
> I've noticed cproc generating what I'm fairly sure are redundant QBE
> cnew instructions.
> 
> Simple test case:
> 
> int
> test(int v)
> {
>     if (v == 45 || v == 47)
>         return 1;
>     return 0;
> }
> 
> Generates:
> 
> export
> function w $test(w %.1) {
> @start.1
>     %.2 =l alloc4 4
>     storew %.1, %.2
> @body.2
>     %.3 =w loadw %.2
>     %.4 =w ceqw %.3, 45
>     jnz %.4, @logic_join.4, @logic_right.3
> @logic_right.3
>     %.5 =w loadw %.2
>     %.6 =w ceqw %.5, 47
>     %.7 =w cnew %.6, 0     <------ THIS !!!
> @logic_join.4
>     %.8 =w phi @body.2 1, @logic_right.3 %.7
>     jnz %.8, @if_true.5, @if_false.6
> @if_true.5
>     ret 1
> @if_false.6
>     ret 0
> }
> 
> I'm fairly sure the cnew marked above is a nop (well copy to be precise).
> 
> I've added QBE code to deal with this in (GVN) copy detection, but
> interested in why?

The reason is that the || operator needs to evaluate to 0 or 1 and
it doesn't have any special case handling for when it's right-hand
expression is *already* 0 or 1. The source of the redundant expression
is

	convert(f, &typebool, e->u.binary.r->type, r)

in qbe.c for TLOR/TLAND. In terms of just the types, the right-hand
side has type int, which has a large range of possible values. We'd
have to look at the specific sub-expression kind to know that the
possible values are only 0 and 1.

For example, consider the following slightly modified condition

	if (v == 45 || v)
		return 1;

Here we get the following:

		%.3 =w loadw %.2
		%.4 =w ceqw %.3, 45
		jnz %.4, @logic_join.4, @logic_right.3
	@logic_right.3
		%.5 =w loadw %.2
		%.6 =w cnew %.5, 0
	@logic_join.4
		%.7 =w phi @body.2 1, @logic_right.3 %.6
		jnz %.7, @if_true.5, @if_false.6

In this case, the cnew %.5, 0 is necessary, otherwise the whole
controlling expression would evaluate to v if the right side of ||
is reached.

It could be possible to omit this if cproc were to look inside the
sub-expressions to see if it were a relational, equality, or logical
operator and skip the cnew. However, in my opinion it's best to
generate code in the simplest way possible and leave the optimizations
to qbe.

BTW, I just want to say it's been really cool to see your work on
QBE. I haven't had a chance to look super closely, but I've been
following with interest from the sidelines. So far, my main goal
with cproc and qbe has been simply to get things to work, I haven't
looked much at performance at all, so it's great to have someone
actually take a look and try to address some of the shortcomings.

Keep up the good work!

> However, in my opinion it's best to
> generate code in the simplest way possible and leave the optimizations
> to qbe.

I tend to agree.

> ... it's great to have someone
> actually take a look and try to address some of the shortcomings.
>
> Keep up the good work!

Thank you!