feat: support configurable constant-fold exclusions - #4450
Conversation
github-actions
Bot
added
component: lowering
d688a7c to
7c18031
Compare
| mark_constant_fold_exclusions, | ||
| ) | ||
|
|
||
| TEST_RULE_ID = "test_arbitrary_node" |
There was a problem hiding this comment.
Registering test_arbitrary_node at import time permanently adds it to the global rule registry. Can we register/unregister so this test node isnt in the registry for other tests?
| incompatible_settings, | ||
| ) | ||
|
|
||
| def test_registered_rule_can_mark_an_arbitrary_node(self): |
There was a problem hiding this comment.
This test verifies that a node gets marked, but not that the marked node survives constant_fold. Can we also run constant_fold and assert the node wasn’t folded away?
| continue | ||
| _mark_constant_fold_exclusion(rule(node), rule_id) | ||
|
|
||
| if disabled_rule_ids: |
There was a problem hiding this comment.
Marks can also be set earlier, and this pass is supposed to revoke them when the rule is disabled. Can we add a test that pre-seeds node.meta with a rule ID and asserts it’s cleared after running with that rule disabled?
|
|
||
| @register_constant_fold_exclusion_rule(TEST_RULE_ID) | ||
| def custom_rule(node): | ||
| return (node,) if node.target is custom_target else () |
There was a problem hiding this comment.
Is it a must for users to register rules?
From the perspective of users, is there any principle for them to say in which scenarios they need to register rules? Can we predefine some rules?
3 participants
Description
This PR introduces a new lowering pass
mark_constant_fold_exclusionsjust before theconstant_foldpass. The new pass is used to mark specific nodes so that they will not be constant folded in the next pass.The feature uses a rule based, configurable approach. There are pre-defined rules, and by default all rules are applied. User can use a
disabled_constant_fold_exclusionssetting to disable one or more rules by specifying the rule name.Currently, this PR should not affect the behavior because the rule list is empty. I will work on add a new rule related to attention in the next PR, which depends on the current PR.
Type of change
Please delete options that are not relevant and/or add your own.
Checklist: