-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move checkConcretization for reshapes #2363
Conversation
This test is actually tougher than the big repro on #2359. It necessitate either removing the check altogether or the path I took which is to concretize Resized to broadcast extents as constant 1 so that we can evaluate max(0, min(i0, 1)) as 1 without calling simplifyExpr. A less invasive solution would be to remove the extent check in `SqueezeOp::checkConcretization`. We could also just remove `Expr::checkConcretization` and `checkConcretizedUses`. They are only used for SqueezeOp currently and are not adding much value anyway probably.
!build --diff |
Confirmed this fixes the thunder benchmark bug. This runs:
|
We actually _can_ squeeze expanded dimensions, which is how we compute reductions of expanded dims.
NVF_CHECK( | ||
!new_id->hasExpandedExtent(), "Can not squeeze expanded dimension(s)."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check is outdated as of #1679 which allowed squeezing expanded dimensions.
!build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Looks like CI is down?! cc'ing @xwang233 |
Seems like a machine issue. I restarted the failing jobs. |
!build |
The included test is the small one provided by @jjsjann123 in #2359 and it's actually tougher than the original repro. It necessitates either removing the check that concretized squeezed extents are constant 1 or to concretize Resized to broadcast extents as constant 1 so that we can evaluate
max(0, min(i0, 1))
asoneVal()
without callingsimplifyExpr
. I went with removing the check, which means in this example we have broadcast dimension with a dynamic shape likemax(0, min(i0, 1))
. Since we're concretizing to Broadcast, we know that dimension is not zero; if it were then we'd concretize toIteration
andSqueezeOp::checkConcretization
would fail the IterType check. Still, I don't love that the expression cannot be simplified so it appears in the kernel (i9
andi10
):If you look closely though,
i10
is not used so it will be DCEd anyway. Still, it might be nice to concretize broadcast extents to 1 just to clean up these expressions if they appear downstream. I tried that hastily but ran into some issues so I'll leave it for another PR.Fixes #2359