Merged
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.
Collaborator
Author
|
!build --diff |
Collaborator
Author
|
Confirmed this fixes the thunder benchmark bug. This runs: |
We actually _can_ squeeze expanded dimensions, which is how we compute reductions of expanded dims.
jacobhinkle
commented
Jun 7, 2024
Comment on lines
-1412
to
-1413
| NVF_CHECK( | ||
| !new_id->hasExpandedExtent(), "Can not squeeze expanded dimension(s)."); |
Collaborator
Author
There was a problem hiding this comment.
This check is outdated as of #1679 which allowed squeezing expanded dimensions.
Collaborator
Author
|
!build |
Collaborator
|
Looks like CI is down?! cc'ing @xwang233 |
Collaborator
Seems like a machine issue. I restarted the failing jobs. |
Collaborator
Author
|
!build |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 toIterationandSqueezeOp::checkConcretizationwould fail the IterType check. Still, I don't love that the expression cannot be simplified so it appears in the kernel (i9andi10):If you look closely though,
i10is 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