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.

Sign up for GitHub

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

fix: change hello_world cmake compiler use -std=c++17 #116

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
+6 −2
Open

fix: change hello_world cmake compiler use -std=c++17 #116

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
73db335
fix: change hello_world cmake compiler use -std=c++17 && argv add const
weedge Mar 21, 2024
7165806
change example helloworld gemma.cpp git tag
weedge Mar 21, 2024
9b7804c
change main use char** argv
weedge Mar 21, 2024
9c66eca
change main use char** argv
weedge Mar 21, 2024
2bdcf6f
remove const argv
weedge Mar 22, 2024
b9829cc
revert hello_world gemma git tag
weedge Mar 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion examples/hello_world/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,10 @@

cmake_minimum_required(VERSION 3.11)
project(hello_world)
set(CMAKE_CXX_STANDARD 17)
set(CMAKE_CXX_STANDARD_REQUIRED ON)
set(CMAKE_EXPORT_COMPILE_COMMANDS ON)
#set(CMAKE_CXX_FLAGS "-Wall -Wextra -fPIC -Wno-unused-parameter")

include(FetchContent)
FetchContent_Declare(highway GIT_REPOSITORY https://github.com/google/highway.git GIT_TAG da250571a45826b21eebbddc1e50d0c1137dee5f)
Expand All @@ -33,7 +36,7 @@ if (BUILD_MODE STREQUAL "local")
# Relative path to gemma.cpp from examples/hello_world/build/
FetchContent_Declare(gemma SOURCE_DIR ../../..)
else()
FetchContent_Declare(gemma GIT_REPOSITORY https://github.com/google/gemma.cpp.git GIT_TAG a9aa63fd2ea6b786ed0706d619588bfe2d43370e)
FetchContent_Declare(gemma GIT_REPOSITORY https://github.com/google/gemma.cpp.git GIT_TAG 73db33580b90f55042285b99139056c632f159bd)
endif()
FetchContent_MakeAvailable(gemma)

Expand Down
4 changes: 2 additions & 2 deletions examples/hello_world/run.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
#include "util/args.h"
// copybara:end
// copybara:import_next_line:gemma_cpp
#include "util/app.h" // LoaderArgs
#include "util/app.h" // LoaderArgs
// copybara:end
#include "hwy/contrib/thread_pool/thread_pool.h"

Expand All @@ -38,7 +38,7 @@ std::vector<int> tokenize(
}

int main(int argc, char** argv) {
gcpp::LoaderArgs loader(argc, argv);
gcpp::LoaderArgs loader(argc, (const char**)argv);
Copy link
Member

Choose a reason for hiding this comment

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

I still do not understand how this change is helpful? :) Const is generally helpful, but for historical reasons we are stuck with non-const for args. It seems like casting is worse than the safety benefit.
We can keep the c++17 change, but let's revert the const or at least move to a separate PR to discuss further :)

Copy link
Author

Choose a reason for hiding this comment

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

yep~ just keep the c++17 change


// Rough heuristic for the number of threads to use
size_t num_threads = static_cast<size_t>(std::clamp(
Expand Down
11 changes: 6 additions & 5 deletions run.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,8 @@ void ShowConfig(LoaderArgs& loader, InferenceArgs& inference, AppArgs& app) {
<< std::thread::hardware_concurrency() << std::endl
<< "Instruction set : "
<< hwy::TargetName(hwy::DispatchedTarget()) << " ("
<< hwy::VectorBytes() * 8 << " bits)" << "\n"
<< hwy::VectorBytes() * 8 << " bits)"
<< "\n"
<< "Compiled config : " << CompiledConfig() << "\n"
<< "Weight Type : "
<< gcpp::TypeName(gcpp::WeightT()) << "\n"
Expand Down Expand Up @@ -277,11 +278,11 @@ int main(int argc, char** argv) {
{
PROFILER_ZONE("Startup.misc");

gcpp::LoaderArgs loader(argc, argv);
gcpp::InferenceArgs inference(argc, argv);
gcpp::AppArgs app(argc, argv);
gcpp::LoaderArgs loader(argc, (const char**)argv);
gcpp::InferenceArgs inference(argc, (const char**)argv);
gcpp::AppArgs app(argc, (const char**)argv);

if (gcpp::HasHelp(argc, argv)) {
if (gcpp::HasHelp(argc, (const char**)argv)) {
ShowHelp(loader, inference, app);
return 0;
}
Expand Down
6 changes: 3 additions & 3 deletions util/app.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ class AppArgs : public ArgsBase<AppArgs> {
}

public:
AppArgs(int argc, char* argv[]) {
AppArgs(int argc, const char* argv[]) {
InitAndParse(argc, argv);
ChooseNumThreads();
}
Expand Down Expand Up @@ -128,7 +128,7 @@ class AppArgs : public ArgsBase<AppArgs> {
};

struct LoaderArgs : public ArgsBase<LoaderArgs> {
LoaderArgs(int argc, char* argv[]) { InitAndParse(argc, argv); }
LoaderArgs(int argc, const char* argv[]) { InitAndParse(argc, argv); }

static std::string ToLower(const std::string& text) {
std::string result = text;
Expand Down Expand Up @@ -205,7 +205,7 @@ struct LoaderArgs : public ArgsBase<LoaderArgs> {
};

struct InferenceArgs : public ArgsBase<InferenceArgs> {
InferenceArgs(int argc, char* argv[]) { InitAndParse(argc, argv); }
InferenceArgs(int argc, const char* argv[]) { InitAndParse(argc, argv); }

size_t max_tokens;
size_t max_generated_tokens;
Expand Down
10 changes: 5 additions & 5 deletions util/args.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ class ArgsBase {
// consider adding a hash-map to speed this up.
class ParseVisitor {
public:
ParseVisitor(int argc, char* argv[]) : argc_(argc), argv_(argv) {}
ParseVisitor(int argc, const char* argv[]) : argc_(argc), argv_(argv) {}

template <typename T>
void operator()(T& t, const char* name, const T& /*init*/,
Expand Down Expand Up @@ -167,7 +167,7 @@ class ArgsBase {
}

int argc_;
char** argv_;
const char** argv_;
}; // ParseVisitor

template <class Visitor>
Expand All @@ -192,19 +192,19 @@ class ArgsBase {
ForEach(visitor);
}

void Parse(int argc, char* argv[]) {
void Parse(int argc, const char* argv[]) {
ParseVisitor visitor(argc, argv);
ForEach(visitor);
}

// For convenience, enables single-line constructor.
void InitAndParse(int argc, char* argv[]) {
void InitAndParse(int argc, const char* argv[]) {
Init();
Parse(argc, argv);
}
};

static inline HWY_MAYBE_UNUSED bool HasHelp(int argc, char* argv[]) {
static inline HWY_MAYBE_UNUSED bool HasHelp(int argc, const char* argv[]) {
// TODO(austinvhuang): handle case insensitivity
if (argc == 1) {
// no arguments - print help
Expand Down