combine ifelse and select_value; improve ifelse t-func
#27068
Conversation
|
Should address the same benchmarks that #26969 was supposed to. @nanosoldier |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan |
|
Do we think this actually introduced allocations in the union benchmarks, or is something up with the benchmarks or the comparison data? |
|
I was about to check. |
|
Looks like something is broken with select_value codegen. It boxes the value in order to do the select: I suspect you need to use the inferred return type in codegen to force an implicit convert of the arguments |
|
I have a fix. Let me push it here. |
Keno
force-pushed
the
jb/ifelse
branch
2 times, most recently
from
598af2c to
e77f2c7
Compare
src/processor.cpp
Outdated
| if (match.best_idx == (uint32_t)-1) { | ||
| match.best_idx = 0; | ||
| //jl_error("Unable to find compatible target in system image."); | ||
| } |
|
Fixed. That's what I get for coding before breakfast. @nanosoldier |
| tx = argtypes[3] | ||
| ty = argtypes[4] | ||
| if isa(fargs[3], Slot) && slot_id(cnd.var) == slot_id(fargs[3]) | ||
| tx = typeintersect(tx, cnd.vtype) |
There was a problem hiding this comment.
just tx = cnd.vtype. it's already guaranteed to be a subtype of the old tx
base/compiler/tfuncs.jl
Outdated
| end | ||
| if !isa(cnd, Conditional) && !(Bool ⊑ cnd) | ||
| elseif isa(cnd, Conditional) | ||
| # handled in abstract_call |
| @@ -3155,15 +3153,15 @@ static jl_cgval_t emit_call(jl_codectx_t &ctx, jl_expr_t *ex) | |||
| } | |||
|
|
|||
| if (f.constant && jl_isa(f.constant, (jl_value_t*)jl_builtin_type)) { | |||
| if (f.constant == jl_builtin_ifelse && nargs == 4) | |||
| return emit_ifelse(ctx, argv[1], argv[2], argv[3], rt); | |||
There was a problem hiding this comment.
should go in emit_builtin_call with the others like it?
| @@ -200,8 +200,11 @@ add_tfunc(Core.Intrinsics.select_value, 3, 3, | |||
| else | |||
| return Bottom | |||
| end | |||
| elseif isa(cnd, Conditional) | |||
| # handled in abstract_call | |||
There was a problem hiding this comment.
it's not fully handled there, so it can appear here too
There was a problem hiding this comment.
Is there anything further that can be done with Conditional here? Anyway I think it's helpful to have this comment so one knows where to look for the full behavior.
There was a problem hiding this comment.
you can check whether the conditional is equivalent to a Const (i.e. that special case we added in).
There was a problem hiding this comment.
Ok. I say we do that later unless it's needed to meet our immediate goals.
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan |
|
Performance looks good. |
Fix ifelse codegen Our return hint may be a concrete type, in which case we want to do the implicit convert before we decide on a strategy for actually emitting the select.
TODO: This regresses some inference support for ifelse, likely undoing some of the work from #27068 ("Combine `ifelse` and `select_value`; improve `ifelse` t-func")
No description provided.