Comment # 18
on bug 106928
from Miroslav Šustek
(In reply to Roland Scheidegger from comment #12) > (In reply to ubizjak from comment #11) > > The (effectively the same patch as yours) proposed patch would be: > > > > diff --git a/src/gallium/drivers/r600/sb/sb_expr.cpp > > b/src/gallium/drivers/r600/sb/sb_expr.cpp > > index 7a5d62c8e8..a609d1377f 100644 > > --- a/src/gallium/drivers/r600/sb/sb_expr.cpp > > +++ b/src/gallium/drivers/r600/sb/sb_expr.cpp > > @@ -714,6 +714,8 @@ bool expr_handler::fold_assoc(alu_node *n) { > > > > n->src.resize(2); > > n->bc.set_op(ALU_OP2_ADD); > > + fold_alu_op2(*n); > > + return true; > > } > > } else if (last_arg >= 0) { > > n->src[0] = a->src[last_arg]; > > > > WDYT? > > I am not quite convinced it's ok to return true (in fold_alu_op3) if the > _expression_ hasn't really been folded. You are quite right that just above it > looks similar, but all other places always return the return value of > fold_alu_op2 when calling into it from fold_alu_op3. > (Not saying it isn't correct, just saying I can't tell...) I guess it is OK. There already is the same pattern in fold_mul_add where it tries to fold: > ADD x, MUL(y, z) into: > MULADD y, z, x and when it succeeds, it does: > > fold_alu_op3(*n); > return true; That means that no matter if fold_alu_op3 folds the instruction any further, fold_mul_add returns true because it converted ADD and MUL into MULADD. In fold_alu_op2 where fold_mul_add is called, the return value is used in an early return condition: > if (n.bc.op == ALU_OP2_ADD) { > if (fold_mul_add(&n)) > return true; > } It seems OK to me to return true after calling fold_alu_op2 in fold_alu_op3 as a sign that the node should not be folded as op3 anymore. Also, the return value of fold_alu_op* is thrown away anyway here: > bool expr_handler::try_fold(value* v) { > assert(!v->gvn_source); > > if (v->def) Here expr_handler::try_fold(node* n) calls the fold_alu_op* methods: > try_fold(v->def); > > if (v->gvn_source) > return true; > > return false; > } I like the patch that ubizjak@gmail.com posted: > diff --git a/src/gallium/drivers/r600/sb/sb_expr.cpp b/src/gallium/drivers/r600/sb/sb_expr.cpp > index 1df78da660..8cbff6f577 100644 > --- a/src/gallium/drivers/r600/sb/sb_expr.cpp > +++ b/src/gallium/drivers/r600/sb/sb_expr.cpp > @@ -723,6 +723,8 @@ bool expr_handler::fold_assoc(alu_node *n) { > > n->src.resize(2); > n->bc.set_op(ALU_OP2_ADD); > + fold_alu_op2(n); > + return true; > } > } else if (last_arg >= 0) { > n->src[0] = a->src[last_arg];
You are receiving this mail because:
- You are the assignee for the bug.
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
