[NNC] IRSimplifier rules for Compare and Mod #46412
Conversation
💊 CI failures summary and remediationsAs of commit 96e664e (more details on the Dr. CI page):
Extra GitHub checks: 1 failed
This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group. This comment has been revised 4 times. |
facebook-github-bot
added
the
oncall: jit
There was a problem hiding this comment.
I think that transformation is not correct if x can be 0.
There was a problem hiding this comment.
This should be guarded by the check on line 1072.
There was a problem hiding this comment.
Oh I see what you mean, we don't know at simplification time if it will hit the 0 % x branch or x % x branch. Good catch. We could replace it with CompareSelect(x, 0, kEq, 0, 1)? Is that better?
There was a problem hiding this comment.
Probably not right. Unfortunate, I guess remove this rule.
There was a problem hiding this comment.
Do we simplify float expressions? For them I think some of these simplifications could be invalid because of NaNs.
There was a problem hiding this comment.
Hmm, thats true also. We could bail out for floating points the same way we do for arithmetic simplification.
There was a problem hiding this comment.
The same concern as @ZolotukhinM pointed out above applies here and in several cases below as well, right? What if x can be 0? Is it okay to simplify such terms in that case?
There was a problem hiding this comment.
I agree for the Mod case since it's reasonable to have zero on the LHS of a Mod (not the RHS). For Div I think it's fine to assume the user is not intentionally dividing by zero, or at least if they are they don't care about the result.
There was a problem hiding this comment.
For Div I think it's fine to assume the user is not intentionally dividing by zero, or at least if they are they don't care about the result.
I'm not entirely sure about this. Imagine I have a[i,j] / a[i,j]: if there are 0 elements in a[i,j] then it'll be either an exception or NaN. If we optimize it to 1, we change the model's behavior.
There was a problem hiding this comment.
Here is an example:
>>> a = torch.tensor([0, 1, 2], dtype=torch.int32)
>>> b = torch.div(a, a)
>>> b
tensor([nan, 1., 1.])
>>> b = torch.floor_divide(a, a)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
RuntimeError: ZeroDivisionError
There was a problem hiding this comment.
I'm trying to think of an actual situation that's important to preserve. Feels a bit like cutting off our nose to spite our face if we prevent an optimization because it might prevent a crash that should occur. Depends if you regard the crash as an important signal I guess.
This would be fixable with a separate zero-check I guess, but that defeats the point of simpliying.
There was a problem hiding this comment.
That code enforces X / 0 = 0 in all paths though, it's DivNoNan, not Div.
I could see adding a DivNoNan operator though.
There was a problem hiding this comment.
@nickgg Fair enough, there is no exact equivalent between PyTorch and TF. PyTorch returns inf on division by 0 whereas TF returns nan and there's difference in behavior between integer and floating point path, iirc.
Not simplifying for now is fair enough, if we let it be we'll return inf I believe, just as eager does.
There was a problem hiding this comment.
@asuhan does Pytorch return inf on integer div by 0? That's interesting. Without simplification I'd expect us to crash.
There was a problem hiding this comment.
@nickgg Yes, because division promotes to float. Integer (floor) division throws a ZeroDivisionError. For floating point tensors, both division and floor division by zero return inf.
Codecov Report
@@ Coverage Diff @@
## master #46412 +/- ##
==========================================
- Coverage 68.39% 68.39% -0.01%
==========================================
Files 411 411
Lines 53972 53972
==========================================
- Hits 36914 36913 -1
- Misses 17058 17059 +1 |
nickgg
force-pushed
the
simplifyCmpModDiv
branch
from
34a29e8 to
96e664e
Compare
nickgg
changed the title
|
I've removed symbolic simplification of Div from this PR. |
There was a problem hiding this comment.
@nickgg has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
💊 CI failures summary and remediationsAs of commit 96e664e (more details on the Dr. CI page):
Extra GitHub checks: 1 failed
This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group. This comment has been revised 3 times. |
5 participants
Adds new rules to the NNC IRSimplifier to take care of the following cases:
Comparisons which are symbolic but have a constant difference. E.g. this is most useful in cases like
if (x > x + 4) ...which we can now eliminate.Simplification of
Modnodes, including simple rules such as0 % xandx % 1, but also factorization of both sides to find common symbolic multiples. E.g.(x * y) % xcan be cancelled out to0.See tests for many more examples!