fix(Inference): Don't add constraints between static edges #693
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #693 +/- ##
=======================================
Coverage 83.93% 83.94%
=======================================
Files 66 66
Lines 12706 12706
Branches 12706 12706
=======================================
+ Hits 10665 10666 +1
+ Misses 1285 1284 -1
Partials 756 756 ☔ View full report in Codecov by Sentry. |
| @@ -482,7 +482,7 @@ mod test { | |||
| FunctionType::new_linear(just_list.clone()).with_extension_delta(&exset), | |||
| )?; | |||
|
|
|||
| let pred_const = cfg.add_constant(ops::Const::unary_unit_sum(), None)?; | |||
| let pred_const = cfg.add_constant(ops::Const::unary_unit_sum(), exset)?; | |||
There was a problem hiding this comment.
This is because we can no longer infer an extension set for the Const itself, right? Presumably we could still use None for the LoadConstant? So what about changing the builder to automatically use pure for the Const node, but still allow specifying something (or None) for the LoadConstant?
There was a problem hiding this comment.
Ah, no, hang on - this is just the Const, there is no LoadConstant here. So IOW, the exset could be anything, that will play no further part in inference?
Hmmm, we should drop the param. That could be another PR/issue, tho.
There was a problem hiding this comment.
I think we should drop the parameter, default to pure for const and move this param to load_constant. I'll open another PR for this.
At the moment though, load_constant uses the extension set that is provided with the ConstId, which is why exset here needs to be specified when the const node is created
|
There is also the question of, if there's a constant value from a particular extension, do we want that to be reflected in the extension-delta of the containing Dfg/Function. This PR means it won't be. That might be fine (if values are passed around as generic YAML, say) or not fine (if we need to understand the extension to load CustomConst implementations)... this is probably worth making an issue? |
That's a good point. I believe the original behaviour is motivated by the latter (black box constants), which is a fair concern. Hence, we should scrap this PR! |
As mentioned in #688, (here) don't require that both sides of a static edge have the same extension requirements, since the static edge often has the empty set.