Skip to content

Separate LoopVectorization.vsum from VectorizationBase.vsum#516

Closed
brenhinkeller wants to merge 2 commits into
JuliaSIMD:mainfrom
brenhinkeller:main
Closed

Separate LoopVectorization.vsum from VectorizationBase.vsum#516
brenhinkeller wants to merge 2 commits into
JuliaSIMD:mainfrom
brenhinkeller:main

Conversation

@brenhinkeller

@brenhinkeller brenhinkeller commented Dec 6, 2023

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

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

@chriselrod

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
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