Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/166065
Note: Links to docs will display an error until the docs builds have been completed. ✅ You can merge normally! (1 Unrelated Failure)As of commit 634d52b with merge base 52e744d ( UNSTABLE - The following job is marked as unstable, possibly due to flakiness on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
pytorch-bot
bot
added
module: cpu
jmaczan
changed the title
Lucaskabela
left a comment
Lucaskabela
left a comment
There was a problem hiding this comment.
This generally looks good but we should add some unit test coverage here and include in summary as testplan :)
|
@Lucaskabela Added a test! What do you mean with include in summary as testplan? |
ezyang
left a comment
ezyang
left a comment
There was a problem hiding this comment.
The ValueRange logic seems to have exactly been copied from bitwise_or which seems unlikely.
|
@ezyang Yes, I didn't understand the concept of upper/lower range analysis, so I tried to use whatever there is for OR and see how tests behave. Now I think I get it and for bitwise xor I try to xor all 4 possible extreme values (lower-lower, lower-upper, upper-lower, upper-upper) and get the lowest of these 4 as lower bound and the highest as upper bound. This works for regular ints. But we can also have infinities and then I don't think we can soundly get a better estimation than -inf/inf, so similarly to I have some unit tests locally for this, but I don't know where to put them in pytorch, because I didn't see any test file for |
|
I don't mind having a very bad bound here, fwiw |
|
@ezyang So I think it’s ready to re-review. |
soulitzer
added
the
triaged
torch/_dynamo/variables/builtin.py
Outdated
| MutableMappingVariable, | ||
| SetVariable, | ||
| UserDefinedDictVariable, | ||
| UserDefinedObjectVariable, |
There was a problem hiding this comment.
This seems like a behavior difference
There was a problem hiding this comment.
Correct, I have reverted that. I have used call_or as a reference, but seems like I shouldn't
torch/utils/_sympy/value_ranges.py
Outdated
|
|
||
| @staticmethod | ||
| def xor_(a, b): | ||
| return ValueRanges.coordinatewise_increasing_map(a, b, sympy.Xor) |
There was a problem hiding this comment.
Pretty sure this is dead? Also, this is not coordinatewise increasing: 0 ^ 1 = 1, but increasing first coord to 1 gives 1 ^ 1 = 0
There was a problem hiding this comment.
Removed. Instead, in bitwise_xor I directly compute lower and upper when a and b are booleans. I use similar code to what I previously assumed I can use for general case for ints, but here I think it's safe to use
Initially, I didn't dig deep enough when coded it and used or_ as a source of truth and tbh this was just incorrect. xor, unlike or and and is less (not at all?) monotonic, and from what I checked coordinatewise_increasing_map assumes that next values always increase, which is not always a case, as you pointed out
torch/utils/_sympy/value_ranges.py
Outdated
| upper_upper = a.upper ^ b.upper | ||
|
|
||
| lower = min(lower_lower, lower_upper, upper_lower, upper_upper) | ||
| upper = max(lower_lower, lower_upper, upper_lower, upper_upper) |
There was a problem hiding this comment.
This seems unlikely to be right. Please reason through this ChatGPT review:
Counterexample:
• Let a ∈ [8, 15], b ∈ [8, 15].
• Your code checks: 8^8=0, 8^15=7, 15^8=7, 15^15=0 → returns range [0, 7].
• But pick interior values: 10 ^ 5 = 15. The true max is at least 15, so [0, 7] is wrong (too tight).
Other issues:
• With negatives, Python’s unbounded two’s-complement semantics make “bit ranges” tricky; the try/except won’t throw, so you’ll quietly produce misleading bounds.
• Even for non-negative ranges, the minimum isn’t determined by corners either (unless some special structure holds).
There was a problem hiding this comment.
@ezyang I have underestimated a difficulty of xor. Now I always use the -inf,inf range, unless there are two booleans or both ranges have same lower and upper, respectively (and aren't infinites). As you mentioned some time ago, it's fine to have too wide bound here, so I think that's ok now. At least it's sound
To optimize the ranges further is a bit rabbit hole, and it seems to be possible but chasing it led me to things like Known Bits Analysis in LLVM (https://llvm.org/docs/GlobalISel/KnownBits.html), which sounds exciting, but it's too big and too difficult for me to implement in this PR
Where I can add tests for bitwise_xor? I didn't find a related test file, can I create a new one?
There was a problem hiding this comment.
You didn't update the bool handling?
Look at test/test_sympy_utils.py
There was a problem hiding this comment.
I've added "bitwise_xor" to be tested in test/test_sympy_utils.py if that's what you're asking about. If that's all we need, then great and pls rereview
…have their lower and upper equal, get rid of my initially assumpted min/max range and bring back original isinstance args in call_xor (I thought I can reuse these from OR but effectively changed the behavior, so bringing it back as ezyang suggests)
|
Ready for a rereview |
|
@ezyang Pls re-review |
torch/utils/_sympy/value_ranges.py
Outdated
| return not vr.is_bool | ||
|
|
||
|
|
||
| def is_sympy_integer(value) -> TypeGuard[sympy.Integer]: |
There was a problem hiding this comment.
Actually should be TypeIs instead of TypeGuard here, we have a stronger invariant.
|
@pytorchbot merge |
|
This PR has pending changes requested. Please address the comments and update the PR before merging. |
…ls lack of many of comparison methods
|
@ezyang pls rerun merge I fixed the tests - impl works the same way but I don't use sympy bools in min/max because it didn't work ( The PR can't be merge also because it has requested changes from @Lucaskabela which probably are not applicable anymore |
|
@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 |
Add missing support for xor (and maybe some other binary ops later on) Fixes pytorch#146688 Pull Request resolved: pytorch#166065 Approved by: https://github.com/ezyang
8 participants
Add missing support for xor (and maybe some other binary ops later on)
Fixes #146688
cc @jgong5 @mingfeima @XiaobingSuper @sanchitintel @ashokei @jingxu10 @jerryzh168 @aditew01 @ezyang @EikanWang @wenzhe-nrv @voznesenskym @penguinwu @Guobing-Chen @zhuhaozhe @blzheng @jiayisunx @chenyang78 @kadeng @chauhang @amjames @Lucaskabela