build: add Makefile for intrinsics compiler workflow#5
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR adds a Makefile to streamline the workflow for building and testing atomix with a customized Go compiler that supports intrinsics. The Makefile automates the installation of the custom compiler, building, testing, and verifying that intrinsics are properly applied.
Changes:
- Added Makefile with 7 targets for managing the intrinsics compiler workflow
- Includes targets for compiler installation, build, test, benchmarking, verification, and cleanup
- Provides comprehensive help documentation for all targets
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| clean: | ||
| rm -f coverage.out | ||
| rm -f test_* | ||
| GOROOT=$(GOROOT_INTRINSIC) $(GO_INTRINSIC) clean -cache 2>/dev/null || true |
There was a problem hiding this comment.
The clean target will fail if the intrinsics compiler is not installed, as it unconditionally uses GO_INTRINSIC which may not exist. The target should check if the compiler exists before attempting to use it, or fall back gracefully. Consider wrapping the go clean command in a conditional check similar to the require-compiler macro.
| GOROOT=$(GOROOT_INTRINSIC) $(GO_INTRINSIC) clean -cache 2>/dev/null || true | |
| @if [ -x "$(GO_INTRINSIC)" ]; then \ | |
| GOROOT=$(GOROOT_INTRINSIC) $(GO_INTRINSIC) clean -cache 2>/dev/null || true; \ | |
| fi |
| @echo "Verifying intrinsics are applied..." | ||
| @echo "" | ||
| @echo "=== Checking for inline atomic instructions ===" |
There was a problem hiding this comment.
The verify target's grep patterns include instructions like "XCHG" and "CMPXCHG" which are x86-64 specific, and "LDADDA", "LDADDAL", "STLR", etc. which are ARM64 specific. This command will only show relevant output on the architecture where it's run. Consider clarifying in the output message that results depend on the current architecture, or add architecture-specific checks to provide clearer feedback.
| @echo "Verifying intrinsics are applied..." | |
| @echo "" | |
| @echo "=== Checking for inline atomic instructions ===" | |
| @echo "Verifying intrinsics are applied (architecture-specific check)..." | |
| @echo "" | |
| @echo "=== Checking for inline atomic instructions ===" | |
| @echo "Note: Patterns include x86-64 (XCHG/CMPXCHG, MOVLstore/MOVQstore) and ARM64 (LDADDA/LDADDAL/STLR/LDAR) instructions; only those relevant to $$(uname -m) will appear." |
| @if [ -d "$(COMPILER_DIR)" ]; then \ | ||
| echo "Updating intrinsics compiler..."; \ | ||
| cd $(COMPILER_DIR) && git fetch origin && git checkout $(COMPILER_BRANCH) && git pull origin $(COMPILER_BRANCH); \ | ||
| else \ | ||
| echo "Cloning intrinsics compiler..."; \ | ||
| mkdir -p $(dir $(COMPILER_DIR)); \ | ||
| git clone --branch $(COMPILER_BRANCH) $(COMPILER_REPO) $(COMPILER_DIR); \ | ||
| fi | ||
| @echo "Building compiler (this may take several minutes)..." | ||
| cd $(COMPILER_DIR)/src && ./make.bash | ||
| @echo "✓ Intrinsics compiler ready at $(GOROOT_INTRINSIC)" |
There was a problem hiding this comment.
The install-compiler target chains multiple git operations with && which means if any step fails, the build step will not run. However, there's no cleanup or error recovery. If the git fetch or checkout fails, the user might be left with a partially updated repository. Consider adding error handling or at least documenting the expected behavior when git operations fail.
| @if [ -d "$(COMPILER_DIR)" ]; then \ | |
| echo "Updating intrinsics compiler..."; \ | |
| cd $(COMPILER_DIR) && git fetch origin && git checkout $(COMPILER_BRANCH) && git pull origin $(COMPILER_BRANCH); \ | |
| else \ | |
| echo "Cloning intrinsics compiler..."; \ | |
| mkdir -p $(dir $(COMPILER_DIR)); \ | |
| git clone --branch $(COMPILER_BRANCH) $(COMPILER_REPO) $(COMPILER_DIR); \ | |
| fi | |
| @echo "Building compiler (this may take several minutes)..." | |
| cd $(COMPILER_DIR)/src && ./make.bash | |
| @echo "✓ Intrinsics compiler ready at $(GOROOT_INTRINSIC)" | |
| @set -e; \ | |
| if [ -d "$(COMPILER_DIR)" ]; then \ | |
| echo "Updating intrinsics compiler..."; \ | |
| cd $(COMPILER_DIR); \ | |
| if ! git fetch origin; then \ | |
| echo "Error: git fetch failed. Repository may be in a partial state at $(COMPILER_DIR)."; \ | |
| exit 1; \ | |
| fi; \ | |
| if ! git checkout $(COMPILER_BRANCH); then \ | |
| echo "Error: git checkout $(COMPILER_BRANCH) failed. Please resolve the repository state manually in $(COMPILER_DIR)."; \ | |
| exit 1; \ | |
| fi; \ | |
| if ! git pull origin $(COMPILER_BRANCH); then \ | |
| echo "Error: git pull origin $(COMPILER_BRANCH) failed. Please resolve the repository state manually in $(COMPILER_DIR)."; \ | |
| exit 1; \ | |
| fi; \ | |
| else \ | |
| echo "Cloning intrinsics compiler..."; \ | |
| mkdir -p $(dir $(COMPILER_DIR)); \ | |
| if ! git clone --branch $(COMPILER_BRANCH) $(COMPILER_REPO) $(COMPILER_DIR); then \ | |
| echo "Error: git clone failed. Cleaning up $(COMPILER_DIR)..."; \ | |
| rm -rf "$(COMPILER_DIR)"; \ | |
| exit 1; \ | |
| fi; \ | |
| fi; \ | |
| echo "Building compiler (this may take several minutes)..."; \ | |
| cd $(COMPILER_DIR)/src; \ | |
| ./make.bash; \ | |
| echo "✓ Intrinsics compiler ready at $(GOROOT_INTRINSIC)" |
| # Configuration | ||
| COMPILER_REPO := https://github.com/hayabusa-cloud/go.git | ||
| COMPILER_BRANCH := atomix | ||
| COMPILER_DIR := $(HOME)/github.com/go |
There was a problem hiding this comment.
The COMPILER_DIR is set to
| COMPILER_DIR := $(HOME)/github.com/go | |
| COMPILER_DIR := $(HOME)/github.com/hayabusa-cloud-go |
| # | ||
| # Prerequisites: | ||
| # - Git | ||
| # - Go 1.25+ (for bootstrapping) |
There was a problem hiding this comment.
The prerequisites comment states "Go 1.25+ (for bootstrapping)" but the CI workflow and README indicate the project requires Go 1.25+. Consider clarifying what minimum Go version is needed for bootstrapping the intrinsics compiler build versus what version is needed to use atomix with the standard Go compiler.
| # - Go 1.25+ (for bootstrapping) | |
| # - Go 1.25+ (required both to bootstrap the intrinsics compiler and to build/test atomix with the standard Go compiler) |
| bench: | ||
| $(require-compiler) | ||
| @echo "Running benchmarks with intrinsics compiler..." | ||
| GOROOT=$(GOROOT_INTRINSIC) $(GO_INTRINSIC) test -bench=. -benchmem ./... |
There was a problem hiding this comment.
The bench target doesn't include any success message or summary output like the other targets (build has "✓ Build successful", test has "✓ Tests passed"). Consider adding a similar success message for consistency, or at least noting that benchmarks completed successfully.
| GOROOT=$(GOROOT_INTRINSIC) $(GO_INTRINSIC) test -bench=. -benchmem ./... | |
| GOROOT=$(GOROOT_INTRINSIC) $(GO_INTRINSIC) test -bench=. -benchmem ./... | |
| @echo "✓ Benchmarks completed" |
| @echo "" | ||
| @echo "=== Checking for inline atomic instructions ===" | ||
| @GOROOT=$(GOROOT_INTRINSIC) $(GO_INTRINSIC) build -gcflags='-S' ./... 2>&1 | \ | ||
| grep -E 'LDADDA|LDADDAL|STLR|STLRW|LDAR|LDARW|XCHG|CMPXCHG|MOVLstore|MOVQstore' | head -20 || true |
There was a problem hiding this comment.
The verify target's grep pattern for atomic instructions is missing common operations like SWPAL (Swap with AcqRel on ARM64) and CASAL (CompareAndSwap with AcqRel on ARM64), which are mentioned in intrinsics.md. According to the example on line 539 of intrinsics.md, the verification should include SWPAL. Consider adding these to provide more comprehensive verification coverage.
| grep -E 'LDADDA|LDADDAL|STLR|STLRW|LDAR|LDARW|XCHG|CMPXCHG|MOVLstore|MOVQstore' | head -20 || true | |
| grep -E 'LDADDA|LDADDAL|STLR|STLRW|LDAR|LDARW|XCHG|CMPXCHG|MOVLstore|MOVQstore|SWPAL|CASAL' | head -20 || true |
| # Prerequisites: | ||
| # - Git | ||
| # - Go 1.25+ (for bootstrapping) | ||
| # - Linux (or WSL2 on Windows) |
There was a problem hiding this comment.
The prerequisite comment states "Linux (or WSL2 on Windows)" but doesn't explain why this limitation exists. The Go compiler can be built on macOS and other Unix-like systems as well. Consider clarifying whether this is a hard requirement or just the tested/supported platforms, and if there's a specific reason for the Linux requirement (e.g., specific build scripts used).
| # - Linux (or WSL2 on Windows) | |
| # - Linux (or WSL2 on Windows) as the tested environment | |
| # (other Unix-like systems, such as macOS, may work but are not officially tested) |
| install-compiler: | ||
| @if [ -d "$(COMPILER_DIR)" ]; then \ | ||
| echo "Updating intrinsics compiler..."; \ | ||
| cd $(COMPILER_DIR) && git fetch origin && git checkout $(COMPILER_BRANCH) && git pull origin $(COMPILER_BRANCH); \ | ||
| else \ | ||
| echo "Cloning intrinsics compiler..."; \ | ||
| mkdir -p $(dir $(COMPILER_DIR)); \ | ||
| git clone --branch $(COMPILER_BRANCH) $(COMPILER_REPO) $(COMPILER_DIR); \ |
There was a problem hiding this comment.
The install-compiler target performs git operations (fetch, checkout, pull) without verifying the authenticity of the fetched code. While the repository URL is hardcoded, consider adding a verification step such as checking a known commit hash or tag, or at least warning users to verify the source, especially since this compiler will be used to build the project.
Summary
install-compiler,build,test,bench,verify,clean,helpTest plan
make helpdisplays all targetsmake buildruns vet then build with intrinsics compilermake testruns tests with race detectionmake verifychecks for inline atomic instructions