Skip to content

Minor build script enhancements #349

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

Merged
merged 1 commit into from
Oct 9, 2024
Merged

Minor build script enhancements #349

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion cmake/imex.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ if (NOT DEFINED IMEX_INCLUDES)

# TODO: Change to main https://github.com/intel/mlir-extensions when all the
# required functionality is merged.
gc_fetch_content(imex "${IMEX_HASH}" https://github.com/intel/mlir-extensions
set(IMEX_URL https://github.com/intel/mlir-extensions)
gc_fetch_content(imex "${IMEX_HASH}" "${IMEX_URL}"
SET IMEX_CHECK_LLVM_VERSION=ON IMEX_ENABLE_L0_RUNTIME=0
)

Expand Down
48 changes: 33 additions & 15 deletions scripts/compile.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,23 @@ $(basename "$0")
[ -r | --release ] Release build, requires rebuild of LLVM in Release mode, activates 'dev' option
[ -c | --clean ] Delete the build artifacts from the previous build
[ -i | --imex ] Compile with IMEX (used for GPU pipeline)
[ -s | --suffix ] Build dir suffix
[ -v | --llvm ] Build llvm only
[ -h | --help ] Print this message
EOF
}

DYN_LINK=OFF
ENABLE_IMEX=false
for arg in "$@"; do
if [ ! -z "$ASSIGN_NEXT" ]; then
eval "$ASSIGN_NEXT=$arg"
unset ASSIGN_NEXT
continue
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

there is better, more transparent and standard way to use this; while [ -v 1 ]; do ; -v is bash specific, one may use -n $1 instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But you have to use shift in each case statement. How is it better? For single value arguments (that we had before) for arg in "$@" seems much less verbose.

case $arg in
-d | --dev)
DEV_BUILD=true
-d | --dev)
DEV_BUILD=true
;;
-i | --imex)
ENABLE_IMEX=true
Expand All @@ -44,12 +51,18 @@ for arg in "$@"; do
-c | --clean)
CLEANUP=true
;;
-s | --suffix)
ASSIGN_NEXT=BUILD_DIR_SFX
;;
-v | --llvm)
LLVM_ONLY=true
;;
-h | --help)
print_usage
exit 0
;;
# -- means the end of the arguments; drop this, and break out of the while loop
*)
*)
echo Unsupported option: $arg
print_usage
exit 1
Expand Down Expand Up @@ -99,6 +112,10 @@ load_llvm() {

build_llvm() {
local llvm_dir="$EXTERNALS_DIR/llvm-project"
LLVM_BUILD_DIR="$llvm_dir/build$BUILD_DIR_SFX"
MLIR_DIR="$LLVM_BUILD_DIR/lib/cmake/mlir"
mkdir -p "$LLVM_BUILD_DIR"

if ! [ -d "$llvm_dir" ]; then
git clone https://github.com/llvm/llvm-project.git
cd "$llvm_dir"
Expand All @@ -111,16 +128,17 @@ build_llvm() {
fi

git checkout ${LLVM_HASH}
[ -z "$CLEANUP" ] || rm -rf build
[ -z "$CLEANUP" ] || rm -rf "$LLVM_BUILD_DIR"

[ "$DYN_LINK" = "OFF" ] && CXX_FLAGS="-fvisibility=hidden"

if [ "$ENABLE_IMEX" = "true" ]; then
# clone IMEX and apply patches
local mlir_ext_dir="$EXTERNALS_DIR/mlir-extensions"
local mlir_ext_dir="$EXTERNALS_DIR/imex-src"
if ! [ -d "$mlir_ext_dir" ]; then
cd "$EXTERNALS_DIR"
git clone https://github.com/intel/mlir-extensions.git
local imex_url="$(grep -F 'set(IMEX_URL ' "$PROJECT_DIR/cmake/imex.cmake" | awk '{print $2}' | tr -d ')')"
git clone "$imex_url" "$mlir_ext_dir"
cd "$mlir_ext_dir"
else
cd "$mlir_ext_dir"
Expand All @@ -134,7 +152,7 @@ build_llvm() {
find "$mlir_ext_dir/build_tools/patches" -name '*.patch' | sort -V | xargs git apply
fi

cmake -G Ninja llvm -B build \
cmake -G Ninja llvm -B "$LLVM_BUILD_DIR" \
-DCMAKE_BUILD_TYPE=$BUILD_TYPE \
-DCMAKE_CXX_FLAGS="$CXX_FLAGS" \
-DCMAKE_CXX_FLAGS_DEBUG="-g -O0" \
Expand All @@ -153,9 +171,7 @@ build_llvm() {
-DLLVM_INSTALL_UTILS=ON \
-DLLVM_INSTALL_GTEST=ON \
-DMLIR_ENABLE_BINDINGS_PYTHON=ON
cmake --build build --parallel $MAX_JOBS

MLIR_DIR="$PWD/build/lib/cmake/mlir"
cmake --build "$LLVM_BUILD_DIR" --parallel $MAX_JOBS
}

# MLIR_DIR is set on all passes
Expand All @@ -174,13 +190,14 @@ get_llvm() {
cd "$llvm_dir"
load_llvm
cd "$PROJECT_DIR"
else
else
MLIR_DIR="$llvm_dir/lib/cmake/mlir"
fi
return 0
}

get_llvm
[ -z "$LLVM_ONLY" ] || exit 0

# written in this form to set LIT_PATH in any case
if ! LIT_PATH=$(which lit) && [ -z "$DEV_BUILD" ]; then
Expand All @@ -193,16 +210,17 @@ if [ -z "$DEV_BUILD" ]; then
FETCH_DIR=$PROJECT_DIR/build/_deps
else
FETCH_DIR=$PROJECT_DIR/externals
LIT_PATH=$PROJECT_DIR/externals/llvm-project/build/bin/llvm-lit
LIT_PATH="$LLVM_BUILD_DIR/bin/llvm-lit"
fi

[ -z "$CLEANUP" ] || rm -rf build
cmake -S . -G Ninja -B build \
BUILD_DIR="$PROJECT_DIR/build$BUILD_DIR_SFX"
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe adding dir suffixes is overengineering as well as the previous attempt to make LLVM location configurable; the common approach is the following: each build environment has exactly the same directories; if you need another environment, you just clone the existing as a whole, this can be done via docker or rsync command; this makes scripts less confusing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With suffixes I can have different builds (debug, release, with imex, without imex) without changing my dev environment. In this way, I can test my changes in different configurations. When creating clones, I also need to populate my changes to each clone, having IDE configurations for each clone, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guessed the intention correctly. My point is the compile script is incorrect tool to implement that logic. You've already have a number of existing tools like docker. We can think together about separate clone_env script, it may use docker or rsync or and git checkout of some dev branch.

Why each environment should look as another? This follows standard linux approach "clone the project to directory and run make test". People expect this, and they never learn your suffixes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suffixes are optional, people are not required to use them. A simple use-case - I've made some changes (not yet committed) in my IDE and want to test with and without IMEX. For this I need not only to build the project with different options, but also to build 2 different versions of LLVM. I just launch the build script from my build env with different suffixes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to have these changes as a part of a separate script?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is definitely possible, the main question - why do we need one more script?

Copy link
Contributor

Choose a reason for hiding this comment

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

everyone uses compile.sh; how many users of compile.sh will be using these suffixes? likely only you; that's separate things we put into compile.sh from the ones we put into some other script

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But creating a separate script just for me does not make sense either.

[ -z "$CLEANUP" ] || rm -rf "$BUILD_DIR"
cmake -S . -G Ninja -B "$BUILD_DIR" \
-DCMAKE_BUILD_TYPE=$BUILD_TYPE \
-DMLIR_DIR=$MLIR_DIR \
-DLLVM_EXTERNAL_LIT=$LIT_PATH \
-DFETCHCONTENT_BASE_DIR=$FETCH_DIR \
-DGC_DEV_LINK_LLVM_DYLIB=$DYN_LINK \
-DGC_ENABLE_IMEX=$ENABLE_IMEX

cmake --build build --parallel $MAX_JOBS
cmake --build "$BUILD_DIR" --parallel $MAX_JOBS