Skip to content
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

Remove deprecated VectorizingIterator module #25150

Merged
merged 3 commits into from
Jun 27, 2024

Conversation

jabraham17
Copy link
Member

@jabraham17 jabraham17 commented May 31, 2024

Removes the deprecated VectorizingIterator module, which has been deprecated for 2 years (deprecation done in #19815)

Note: should be merged after #25143

Testing:

  • Built and checked docs
  • paratest
  • paratest with gasnet

[Reviewed by @lydia-duncan]

Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
Copy link
Member

@lydia-duncan lydia-duncan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you know the pragma is unused? I see you've removed independent tests of it in addition to the uses in the standard module but that doesn't necessarily mean some user isn't relying on it. Did you check it isn't used in Arkouda and CHAMPS? Even though pragmas are unstable in general, it might be kinder to deprecate the pragma before removing it, to at least give users a chance to complain if they were relying on it.

I suspect there probably aren't any uses in the wild and it's safe to remove, but I'm mostly hoping to avoid us establishing that precedent for things that users are relying on. This is something that was at least initially motivated by performance and while I would hope we have better ways of gaining that performance today, that isn't necessarily true and it would be unfortunate to take performance away from users without replacement.

compiler/resolution/lowerIterators.cpp Outdated Show resolved Hide resolved
@jabraham17
Copy link
Member Author

How do you know the pragma is unused? I see you've removed independent tests of it in addition to the uses in the standard module but that doesn't necessarily mean some user isn't relying on it. Did you check it isn't used in Arkouda and CHAMPS? Even though pragmas are unstable in general, it might be kinder to deprecate the pragma before removing it, to at least give users a chance to complain if they were relying on it.

I assumed that there were no uses of the pragma outside our standard library, as my understanding is that pragmas are internal features. Its the same as if a user was relying on a __primitive or chpl_... symbol in an internal module. They aren't meant to be user facing features. The only uses of the pragma I saw where to implement vectorizeOnly and tests of the pragma itself.

I did just check that both CHAMPS and Arkouda do not use this pragma, and I doubt there are other uses of it.

All of that said, out of an abundance of caution, I can undo the removal of the pragma in this PR if you prefer. I don't think its critical to removing VectorizingIterator, I just did it to cleanup after doing so.

@lydia-duncan
Copy link
Member

lydia-duncan commented Jun 27, 2024

They are internal features, but they existed before the external facing attributes feature was added and we definitely haven't done a pass over them to make sure any pragmas that would make sense as attributes get converted over into attributes. I suspect we may have advised users to use them in the distant past (though one would hope that anything falling under that category would have gotten converted to a nicer feature, like a module with functions the user could call ... which is to say that probably the lack of use of VectorizingIterator is a sign that they aren't used heavily elsewhere, given the strong connection between the two.)

I'm not going to push for it too hard, I trust your judgment here. I would still advise cautious around the removal of other pragmas in the future for the reason I outlined, though.

Signed-off-by: Jade Abraham <jade.abraham@hpe.com>
@jabraham17
Copy link
Member Author

Per our offline conversation, I have reverted the pragma removal

Copy link
Member

@lydia-duncan lydia-duncan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks :)

@jabraham17 jabraham17 merged commit 9f89ff1 into chapel-lang:main Jun 27, 2024
7 checks passed
@jabraham17 jabraham17 deleted the remove-VectorizingIterator branch June 27, 2024 22:46
@bradcray
Copy link
Member

Thanks for taking care of this, Jade!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants