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

Separate LoopVectorization.vsum from VectorizationBase.vsum #516

Closed
wants to merge 2 commits into from
Closed

Separate LoopVectorization.vsum from VectorizationBase.vsum #516

wants to merge 2 commits into from

Conversation

brenhinkeller
Copy link
Contributor

@brenhinkeller brenhinkeller commented Dec 6, 2023

Seems like it might be better to have LoopVectorization.vsum != VectorizationBase.vsum esp. if we might want to extend the former (but not the latter) in other packages. I think this isn't technically breaking and hopefully it doesn't break anything if I did it right, but will see how tests look

@chriselrod
Copy link
Member

Thanks for addressing this/taking over from #515.
Note that LoopVectorization itself calls VectorizationBase.vsum.
Double check whether it namespacing them properly (VectorizationBase.vsum), or being lazy (LoopVectorization.vsum and relying on the using VectorizationBase: vsum that you removed here).

@brenhinkeller
Copy link
Contributor Author

Oh oops I hadn’t noticed #515! Looks like this is still breaking something, but I don't think I understand why..

@chriselrod
Copy link
Member

Oh oops I hadn’t noticed #515! Looks like this is still breaking something, but I don't think I understand why..

Some failures are because LV uses VectorizationBase.vsum, without saying VectorizationBase.vsum.

@brenhinkeller
Copy link
Contributor Author

That would make sense but I can't find any other usages of vsum when I search the package...

@chriselrod
Copy link
Member

That would make sense but I can't find any other usages of vsum when I search the package...

E.g.

reduction_to_scalar(x::Float64) =
if x == ADDITIVE_IN_REDUCTIONS
:vsum

reduction_to_scalar(op::Operation)::GlobalRef =
lv(reduction_to_scalar(instruction(op)))

LV doesn't call the code, it generates expressions with calls.

@brenhinkeller
Copy link
Contributor Author

Ooh, so is it as simple as replacing that with :(VectorizationBase.vsum) or such? Trying that locally now and looks a bit better but still some local test failures

@chriselrod
Copy link
Member

Ooh, so is it as simple as replacing that with :(VectorizationBase.vsum) or such? Trying that locally now and looks a bit better but still some local test failures

Note that lv(x) (called in one of those code snippets) does GlobalRef(LoopVectorization, x). We need VectorizationBase instead.

@brenhinkeller brenhinkeller closed this by deleting the head repository Jan 7, 2024
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants