bug[next]: Fix CSE inside stencil #1793
Conversation
| @@ -26,17 +26,9 @@ def offset_provider_type(request): | |||
|
|
|||
|
|
|||
| def test_trivial(): | |||
There was a problem hiding this comment.
Just a cleanup, not important for this PR.
| @@ -429,9 +429,9 @@ def apply( | |||
| return cls().visit(node, within_stencil=within_stencil) | |||
|
|
|||
| def generic_visit(self, node, **kwargs): | |||
| if cpm.is_call_to("as_fieldop", node): | |||
| if cpm.is_call_to(node, "as_fieldop"): | |||
There was a problem hiding this comment.
Thinking about the interface, I think the other version would make more sense, because a curried version makes more sense (and therefore works nicer with functools.partial).
There was a problem hiding this comment.
Maybe we can use the __getattr__ to implement cpm.is_call_to.as_fieldop(node)?
There was a problem hiding this comment.
Obviously given my typo I find the other way (node last) also natural (even though that is different day-to-day). However I would argue that the existing ordering is the correct one in python since isinstance and issubclass also use this ordering.
isinstance(obj, type)
The reasoning, I assume, comes from the ordering in natural language: is OBJ of kind KIND -> function is called is_obj_kind. With respect to the cpm.is_call_to.as_fieldop, I think this is a violation of KISS.
There was a problem hiding this comment.
Agree, the missing placeholder <>_is_call_to(<>) in the beginning is the part that makes it ambiguous, and it's just a convention. Following isinstance makes sense.
I thought we cannot easily detect the error because I thought we also accept str as node and promote it to a Sym, like in im, but since that is not the case an assert isinstance(node, itir.Node) is probably good enough.
The common subexpression elimination did not work at all inside of stencils due to a typo. Fixed & added a test to cover this.