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

Update Makefile for HIPBLAS #4

Closed
wants to merge 1 commit into from

Conversation

YellowRoseCx
Copy link
Collaborator

This update

  • changes the DMMV_Y variable to the newer MMV_Y variable
  • exposes some extra options for the user to change such as "LLAMA_CUDA_KQUANTS_ITER" and "LLAMA_CUDA_KQUANTS_ITER"
  • sets MMV_Y variable to any DMMV_Y just in case for backwards compatibility
  • sets CUDA_FORCE_DMMV to 'true' after observing output and reports of garbled output
  • moves the ggml-cuda.o: CXXFLAGS into ifdef statements

Tested and working on latest build on a 6800xt

@SlyEcho
Copy link
Owner

SlyEcho commented Jul 13, 2023

Adding it to CXXFLAGS like that will define those for all of the code instead of just ggm-cuda.cu, the CUDA version uses a different variable so it is isolated, but what was the problem with target-specific variables?

else
CXXFLAGS += -DGGML_CUDA_DMMV_X=32
endif
ifeq ($(LLAMA_CUDA_FORCE_DMMV), true)
Copy link
Owner

Choose a reason for hiding this comment

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

We should keep it the same as the CUDA version:

Suggested change
ifeq ($(LLAMA_CUDA_FORCE_DMMV), true)
ifdef LLAMA_CUDA_FORCE_DMMV

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How would using ifdef work with a true/false boolean? The upstream makefile for CuBLAS doesn't look complete or correct

Copy link
Owner

Choose a reason for hiding this comment

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

Well, it's usually defined to be anything to turn it on, not true or false.

@YellowRoseCx
Copy link
Collaborator Author

what was the problem with target-specific variables

What target-specific variables?

@SlyEcho
Copy link
Owner

SlyEcho commented Jul 13, 2023

It used to be:

LLAMA_CUDA_DMMV_X ?= 32
ggml-cuda.o: CXXFLAGS += -DGGML_CUDA_DMMV_X=$(LLAMA_CUDA_DMMV_X)

That means that when making the target ggml-cuda.o the CXXFLAGS will have these added definitions. But when making some other target, like common.o for example, these definitions will not be present.

The CUDA version uses this:

ifdef LLAMA_CUDA_DMMV_X
	NVCCFLAGS += -DGGML_CUDA_DMMV_X=$(LLAMA_CUDA_DMMV_X)
else
	NVCCFLAGS += -DGGML_CUDA_DMMV_X=32
endif # LLAMA_CUDA_DMMV_X

Well, it does the same thing but in 5 lines. But the difference here is that it uses a different Make variable NVCCFLAGS, which should only be used on the ggml-cuda.cu file as well.

So maybe we can use a different variable like HIPFLAGS or something?

@SlyEcho
Copy link
Owner

SlyEcho commented Jul 13, 2023

I think I got the variable changes working now except for the backwards compatibility with LLAMA_CUDA_DMMV_Y

@SlyEcho SlyEcho closed this Jul 13, 2023
@YellowRoseCx YellowRoseCx deleted the patch-2 branch April 10, 2024 15:15
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