Skip to content

Conversation

@FirstTimeEZ
Copy link
Contributor

@FirstTimeEZ FirstTimeEZ commented Nov 16, 2024

src1 can go down the first pipeline as nullptr and src0 only needs to be checked once

this means the assertion is only required to check if the type is GGML_TYPE_F16 and can usually be skipped
@FirstTimeEZ FirstTimeEZ changed the title vulkan: change an assertion vulkan: change an assertion and minify others Nov 17, 2024
@0cc4m
Copy link
Collaborator

0cc4m commented Nov 17, 2024

"IDE told me to" isn't by itself enough to warrant a change, in my opinion. Especially Visual Studio and MSVC seem to be very quick to warn about potential issues.

If you have a good reason (performance, safety, readability, etc) to refactor code, that's different. But I don't really see that here.

Combining makes the assertions harder to read and at least one of your changes isn't even correct. I'd have to put in some work to check all of them, and I don't see the benefit.

@FirstTimeEZ FirstTimeEZ reopened this Nov 17, 2024
@ggml-org ggml-org locked as too heated and limited conversation to collaborators Nov 17, 2024
@slaren
Copy link
Member

slaren commented Nov 17, 2024

@FirstTimeEZ enough, take a break.

@0cc4m
Copy link
Collaborator

0cc4m commented Nov 18, 2024

You caught the problem I saw, so the assertion changes might be correct now. However, combining assertions is a style decision, not a performance decision, and I prefer to keep them as is. None of the changes you made have "obvious performance reasons".

Improving the logic for SOFT_MAX pipeline selection to remove the assertion there is different, I would accept that change on its own. So if you clean up this PR to only do that, as it was when you opened it, I'll check it and merge it once it's ready.

But let me also say that insulting me is not okay. Deleting or editing your comments doesn't mean I don't see what you wrote. I don't have a personal issue with you, I'm not clueless and my comments on your PR aren't stupid. I wrote most of the Vulkan backend and it's on me to make sure it keeps working.

I should have put the issue I found into my comment, but then again you could have also just asked and given me a little time to respond.

@ggml-org ggml-org unlocked this conversation Nov 18, 2024
@FirstTimeEZ FirstTimeEZ closed this by deleting the head repository Nov 18, 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.

3 participants