-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
[stdlib] Add UInt __invert__
#3643
base: nightly
Are you sure you want to change the base?
Conversation
I believe this won't compile if you add a test case as we can't convert a negative |
41924d0
to
f8807e5
Compare
Signed-off-by: martinvuyk <[email protected]>
Co-authored-by: soraros <[email protected]> Signed-off-by: martinvuyk <[email protected]> Signed-off-by: martinvuyk <[email protected]>
Signed-off-by: martinvuyk <[email protected]>
f8807e5
to
13b1495
Compare
This looks good, although I wonder why |
Hi @helehex , well I think it's just an xor however you express it 🤷♂️ . I just want the functionality to do stuff with hashes |
Yeah it's just a question about consistency. Hopefully this can get merged soon since it's been missing for a while. |
Maybe you should add a test (even though int doesn't have one). Also, if you use mlir ops, the implementation could be the same between return __mlir_op.`index.xor`(
value.value,
__mlir_op.`index.constant`[value = __mlir_attr.`-1:index`](),
) works for |
Honestly this makes no difference, it's trivial. Inverting bits doesn't need a test IMO, that would be testing the CPU basically.
When #3753 gets merged it will fail. And the code is already what gets added by the xor operation @jackos can we merge this directly? |
Both It may be trivial, but having a test is nice regardless to make sure it compiles, and to ensure nothing else breaks it down the line. |
That being said, I'd be fine with it getting merged as is, it's just some suggestions. |
yeah you're right I got confused and thought I changed the mlir index constructor in that PR but it was just the Int one.
Yeah it would be nice, I'm just a bit frustrated and I don't want to extend this PR further |
Hi @martinvuyk that one is already failing a bunch of internal tests, I'll leave comments in that PR. But this is separate I'll add a test on the internal PR and see if all the other internal tests pass. |
!sync |
Add UInt
__invert__