islpy icon indicating copy to clipboard operation
islpy copied to clipboard

Stop using pybind's implicit `__hash__`

Open inducer opened this issue 2 years ago • 6 comments

inducer avatar Dec 30 '22 20:12 inducer

The SSL certificate on https://gmplib.org/ is broken, breaking all the wheel builds. It looks like they're aware of it.

inducer avatar Dec 30 '22 21:12 inducer

Thanks! Looks like most of loopy's errors come from islpy.Space being unhashable. Maybe we can overload in islpy/__init__.py as:

def _isl_space_get_hash(x):
    return hash((x.dim()), x.params()))
Space.__hash__ = _isl_space_get_hash

It is unlikely that ISL will accept a corresponding native function as this defines new private API without any users.

kaushikcfd avatar Dec 31 '22 11:12 kaushikcfd

@isuruf Was there a big reason why we used gmp instead of imath-32 in the wheel? I'm getting ready to drop gmp here because I'm tired of waiting for them to fix their certificate.

inducer avatar Jan 01 '23 20:01 inducer

So... it turns out that this is what has kept https://github.com/inducer/loopy/issues/576 from biting as hard as it deserved to.

https://gist.github.com/inducer/9eb97cb5a6064baee63d0ef64233ee3b

together with this patch to loopy illustrates the issue:

diff --git a/loopy/kernel/tools.py b/loopy/kernel/tools.py
index 99f5f350..32e11eba 100644
--- a/loopy/kernel/tools.py
+++ b/loopy/kernel/tools.py
@@ -355,6 +355,7 @@ class SetOperationCacheManager:
 
         for bkt_set, result in bucket:
             if set_.plain_is_equal(bkt_set):
+                assert result == op(set_, *args)
                 return result
 
         result = op(set_, *args)

inducer avatar Jan 02 '23 01:01 inducer

I was thinking I could make a quick PR to loopy which would also add the condition bkt_set.get_var_dict() == set_.get_var_dict(), but the problem is deeper as https://github.com/inducer/loopy/issues/576 points :/. I guess decoupling loopy's ISL usage from relying on dim-names seems the only (practical) option. The other (dirty) option is to make loopy depend on a different islpy that overrides isl_xxx_is_equal.

kaushikcfd avatar Jan 02 '23 02:01 kaushikcfd

@isuruf Was there a big reason why we used gmp instead of imath-32 in the wheel? I'm getting ready to drop gmp here because I'm tired of waiting for them to fix their certificate.

Nope. gmp was faster than imath (no small-integer optimization), but imath-32 (with small integer optimization) should be better.

isuruf avatar Jan 02 '23 04:01 isuruf