Add more types to transform.iname#978
Conversation
alexfikl
force-pushed
the
type-transform-iname
branch
from
4c5c4e1 to
7ee0c81
Compare
loopy/transform/iname.py
Outdated
| # FIXME: parse_match never returns None, so this code is not executed? | ||
| if within is None: | ||
| new_domain = new_domain.project_out(iname_dt, iname_idx, 1) |
There was a problem hiding this comment.
I can't even remember what the intent of this was. Delete.
There was a problem hiding this comment.
| return find_unused_axis_tag(kernel, "g") | ||
| return [find_unused_axis_tag(kernel, "g")] | ||
| elif tag == "unused.l": |
There was a problem hiding this comment.
It doesn't seem like this was ever hit? It returned a single element instead of a list, from what I can tell.
There was a problem hiding this comment.
Untested, undocumented, not functioning? Delete!
There was a problem hiding this comment.
This seems to be documented at https://documen.tician.de/loopy/ref_kernel.html#iname-implementation-tags, but not used or tested anywhere that I could find. Still remove?
There was a problem hiding this comment.
Also, from a bit of git blame, they were just forgotten in a recent-ish change: cafc5ab
loopy/transform/iname.py
Outdated
| knl_inames[sub_iname] = knl_inames[sub_iname].tagged(new_tag) | ||
|
|
||
| return kernel.copy(inames=knl_inames) | ||
| return kernel.copy(inames=constantdict(knl_inames)) |
There was a problem hiding this comment.
Is the kernel supposed to be more immutable? I added these in a few places, but not sure if it's a good idea.
There was a problem hiding this comment.
Hm, just briefly looked at some failures, and it doesn't seem like it is 😁
There was a problem hiding this comment.
Hm, one fun failure with these things: passed in tuple(domains) in a few places which made it fail some tests because [1, 2, 3] != (1, 2, 3) :(
There was a problem hiding this comment.
Morally, LoopKernel is supposed to be immutable. But the data structure still isn't, in places. Any step towards that is welcome though, as long as it doesn't break backwards compatbility.
There was a problem hiding this comment.
I think this might brake compatibility though. Might be better to start with more warnings in __post_init__ or something?
For this, constantdict.copy returns another constantdict, so any places that tries to do that and modify will break. Fortunately constantdict({"a": 1}) == {"a": 1} (for better or worse), so this didn't see too many failures on the CI at least.
There was a problem hiding this comment.
I wasn't saying we should do this now. :)
alexfikl
force-pushed
the
type-transform-iname
branch
2 times, most recently
from
bfe11ea to
a53e22b
Compare
alexfikl
force-pushed
the
type-transform-iname
branch
from
a53e22b to
82f35c2
Compare
loopy/transform/iname.py
Outdated
| # FIXME: parse_match never returns None, so this code is not executed? | ||
| if within is None: | ||
| new_domain = new_domain.project_out(iname_dt, iname_idx, 1) |
There was a problem hiding this comment.
I can't even remember what the intent of this was. Delete.
loopy/transform/iname.py
Outdated
| | Sequence[tuple[str, _Tags_ish]] | ||
| | str), | ||
| *, | ||
| # FIXME: 'force' is unused? |
There was a problem hiding this comment.
Kill it and see what breaks? Or at least deprecate?
There was a problem hiding this comment.
Added a deprecation warning. Seems to have been unused for a long time now: 8734898#diff-a91a39471a27d37ceb2117f72be493a2ab46af1987edc52ff7a432e2c296f2cc.
| return find_unused_axis_tag(kernel, "g") | ||
| return [find_unused_axis_tag(kernel, "g")] | ||
| elif tag == "unused.l": |
There was a problem hiding this comment.
Untested, undocumented, not functioning? Delete!
loopy/transform/iname.py
Outdated
| knl_inames[sub_iname] = knl_inames[sub_iname].tagged(new_tag) | ||
|
|
||
| return kernel.copy(inames=knl_inames) | ||
| return kernel.copy(inames=constantdict(knl_inames)) |
There was a problem hiding this comment.
Morally, LoopKernel is supposed to be immutable. But the data structure still isn't, in places. Any step towards that is welcome though, as long as it doesn't break backwards compatbility.
alexfikl
force-pushed
the
type-transform-iname
branch
4 times, most recently
from
3999c37 to
3d6bc8a
Compare
alexfikl
force-pushed
the
type-transform-iname
branch
from
3d6bc8a to
8228fdb
Compare
|
@inducer Any idea why the grudge CI is unhappy? It seems to pass just fine locally and I can't see any particular changes that should have suddenly broken it.. |
|
News regarding the grudge failures: inducer/grudge#396 |
alexfikl
force-pushed
the
type-transform-iname
branch
from
8228fdb to
ae1f9de
Compare
|
Thx! |
2 participants
This also does some cleanup: updates docs a bit, ports to more f-strings, etc.