gh-128760: Merge subsequent escaping calls to avoid redundant stack spills when escaping calls are separated by comments #128761
Conversation
WolframAlph
commented
Jan 12, 2025
•
WolframAlph
commented
- Issue: Redundant stack spills on subsequent escaping calls separated by comments #128760
|
@picnixz can you take a look? |
|
Can you first resolve the conflicts and make the CI green for this one since it affects generated code? It seems we're indeed doing unnecessary operations, so it's good to fix this. I'd recommend also asking @Eclips4 or @iritkatriel to have a look once it's ready as Mark may not have the bandwidth. |
|
Now, not sure if it's worth a NEWS entry or not. We could gain a bit of performance but only if we have such comments in the code which I don't know whether we do. |
|
I will resolve conflicts and regenerate cases so we will see if there are such cases |
WolframAlph
force-pushed
the
fix-stmt-end
branch
from
ae3c73f to
b8ccca8
Compare
WolframAlph
force-pushed
the
fix-stmt-end
branch
from
187fe01 to
40c91a8
Compare
|
Okay so after resolving conflicts and regenerating cases I noticed that there are changes to those files. No performance related but there is weird reordering of reloading stack & subsequent comment. (You can see this in 8afca3fd). So after some debugging I realized that my initial fix probably should be revisited since it introduces this awkward side effect. In the end I used different approach to fix this. Basically old way of finding escaping calls boundaries but with 1 step in between of merging subsequent calls to avoid redundant spills. More robust solution that fixes initial issue with no side effects. |
WolframAlph
changed the title
|
I'll take a look at it later today, sorry for the delay! |
|
This introduces some amount of complexity for a use case that we dont currently have. So I'm wondering if it's worth it. |
This is the reason why I asked for your opinion 🙂. We might encounter something like this in the future, so the question in my mind is: "Is it worth introducing complexity into the analyzer?" In my very personal opinion, yes, it is worth it. I guess you and @markshannon should decide whether it's worthwhile or not, since both of you have a lot of experience with |
|
I don't think this adds a lot of complexity, but we may run into this at some point so it's worth the fix IMO. |
|
Just checking in, @Fidget-Spinner do you want to merge or close this PR then? |
|
I think we should only introduce this PR when we bump into it as an actual problem. So could you close the PR please but keep the branch? |
|
Closing per request. |
