Ensure that symbolic variables incorporate fresh constraints before they're used #87254
Conversation
…hey're used [ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/87254
Note: Links to docs will display an error until the docs builds have been completed. ✅ No Failures, 5 PendingAs of commit 0587d31: This comment was automatically generated by Dr. CI and updates every 15 minutes. |
github-actions
bot
requested review from
albanD,
anjali411,
antoniojkim,
bdhirsh,
ezyang,
Krovatkin,
miladm and
wconstab
…ts before they're used" [ghstack-poisoned]
| def wrap(self, num): | ||
| return PySymInt(sympy.Integer(num), self.shape_env, constant=num) | ||
|
|
||
| def clone(self): | ||
| return PySymInt(self.expr, self.shape_env, constant=self.constant) | ||
|
|
||
| def _update_expr(self): | ||
| self._expr = self.shape_env.simplify(self._expr) |
There was a problem hiding this comment.
do you think this will noticeably slow down compilation times 🤔. I hope simplify() is a fast no-op
There was a problem hiding this comment.
We have a bunch of caches, so it'll never get run twice.
There was a problem hiding this comment.
Not sure about that? We'd have to audit all expr call sites no?
There was a problem hiding this comment.
What do you mean? I'm saying that self.shape_env.simplify sits behind a LRU cache, so if we hit it twice it should mostly be a no-op.
…ts before they're used" [ghstack-poisoned]
…ts before they're used" [ghstack-poisoned]
| @@ -3801,25 +3801,6 @@ def forward(x): | |||
| ] | |||
| self.common(forward, args) | |||
|
|
|||
| @unittest.skip("https://github.com/pytorch/torchdynamo/issues/1297") | |||
There was a problem hiding this comment.
Removing this since redundant test.
| @@ -45,7 +45,7 @@ def __init__(self, shape_env=None): | |||
| self.shape_env = shape_env | |||
| self.var_to_val = self.shape_env.var_to_val | |||
| self.guards = [] | |||
| self.replacements: Dict[sympy.Symbol, Expr] = {} | |||
| self.replacements: Dict[sympy.Symbol, Expr] = self.shape_env.replacements | |||
There was a problem hiding this comment.
do you want an alias, or a copy?
There was a problem hiding this comment.
might be worth a comment, as the Dict type annot is mutable
|
I'm not qualified to review the inductor code |
|
@pytorchbot merge |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
|
Hey @Chillee. |
Stack from ghstack (oldest at bottom):
cc @jansel @lezcano @fdrocha