combine ifelse and select_value; improve ifelse t-func#27068
combine ifelse and select_value; improve ifelse t-func#27068JeffBezanson merged 2 commits intomasterfrom
ifelse and select_value; improve ifelse t-func#27068Conversation
|
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. |
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
| end | ||
| if !isa(cnd, Conditional) && !(Bool ⊑ cnd) | ||
| elseif isa(cnd, Conditional) | ||
| # handled in abstract_call |
|
|
||
| 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?
| 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.