Re: [gentoo-dev] [PATCH v2 1/2] rocm.eclass: new eclass

[Ulrich Mueller]

>>>>> On Mon, 15 Aug 2022, Yiyang Wu wrote:

> diff --git a/eclass/rocm.eclass b/eclass/rocm.eclass
> new file mode 100644
> index 000000000000..8ca2c051ddce
> --- /dev/null
> +++ b/eclass/rocm.eclass
> @@ -0,0 +1,275 @@
> +# Copyright 2022 Gentoo Authors
> +# Distributed under the terms of the GNU General Public License v2
> +
> +# @ECLASS: rocm.eclass
> +# @MAINTAINER:
> +# Gentoo Science Project <sci@g.o>
> +# @AUTHOR:
> +# Yiyang Wu <xgreenlandforwyy@×××××.com>
> +# @SUPPORTED_EAPIS: 7 8
> +# @BLURB: Common functions and variables for ROCm packages written in HIP
> +# @DESCRIPTION:
> +# ROCm packages such as sci-libs/<roc|hip>* can utilize functions in this eclass.
> +# Currently, it handles the AMDGPU_TARGETS variable via USE_EXPAND, so user can
> +# use USE flag to control which GPU architecture to compile, and ensure coherence
> +# among dependencies. It also specify CXX=hipcc, to let hipcc compile. Another
> +# important function is src_test, which checks whether a valid KFD device exists
> +# for testing, and then execute the test program.

Please wrap lines at below 80 character positions. (This applies both to
documentation and to code.)

> +#
> +# Most ROCm packages use cmake as build system, so this eclass does not export
> +# phase functions which overwrites the phase functions in cmake.eclass. Ebuild
> +# should explicitly call rocm_src_* in src_configure and src_test.
> +#
> +# @EXAMPLE:
> +# # Example for ROCm packages in https://github.com/ROCmSoftwarePlatform
> +# inherit cmake rocm
> +# SRC_URI="https://github.com/ROCmSoftwarePlatform/${PN}/archive/rocm-${PV}.tar.gz -> ${P}.tar.gz"
> +# SLOT="0/$(ver_cut 1-2)"
> +# IUSE="test"
> +# REQUIRED_USE="${ROCM_REQUIRED_USE}"
> +# RESTRICT="!test? ( test )"
> +#
> +# RDEPEND="
> +#     dev-util/hip
> +#     sci-libs/rocBLAS:${SLOT}[${ROCM_USEDEP}]
> +# "
> +#
> +# S=${WORKDIR}/${PN}-rocm-${PV}
> +#
> +# src_configure() {
> +#     local mycmakeargs=(
> +#         -DBUILD_CLIENTS_TESTS=$(usex test ON OFF)
> +#     )
> +#     rocm_src_configure
> +# }
> +#
> +# src_test() {
> +#     rocm_src_test
> +# }
> +#
> +# # Example for packages depend on ROCm libraries -- a package depend on
> +# # rocBLAS, and use comma seperated ${HCC_AMDGPU_TARGET} to determine GPU
> +# # architecture to compile. Requires ROCm version >5.
> +# ROCM_VERSION=5
> +# inherit rocm
> +# IUSE="rocm"
> +# REQUIRED_USE="rocm? ( ${ROCM_REQUIRED_USE} )"
> +# DEPEND="rocm? ( >=dev-util/hip-${ROCM_VERSION}
> +#     >=sci-libs/rocBLAS-${ROCM_VERSION}[${ROCM_USEDEP}] )"
> +# ....
> +# src_configure() {
> +#     if use rocm; then
> +#         local AMDGPU_FLAGS=$(get_amdgpu_flags)
> +#         export HCC_AMDGPU_TARGET=${AMDGPU_FLAGS//;/,}
> +#     fi
> +#     default
> +# }

@EXAMPLE is read as a paragraph, i.e. lines will be wrapped and this is
how it will look when rendered as eclass manpage:

       #  Example for ROCm packages in https://github.com/ROCmSoftwarePlatform
       inherit   cmake   rocm    SRC_URI="https://github.com/ROCmSoftwarePlat‐
       form/${PN}/archive/rocm-${PV}.tar.gz  -> ${P}.tar.gz" SLOT="0/$(ver_cut
       1-2)" IUSE="test" REQUIRED_USE="${ROCM_REQUIRED_USE}"  RESTRICT="!test?
       ( test )"

       [...]

So, you may want to include the whole example in a pair of @CODE tokens.

> +
> +if [[ ! ${_ROCM_ECLASS} ]]; then
> +
> +case "${EAPI:-0}" in

This should be just ${EAPI} without a fallback to 0. Also, the
quotation marks are not necessary.

> +	7|8)
> +		;;
> +	*)
> +		die "Unsupported EAPI=${EAPI} for ${ECLASS}"

Whereas here it should be ${EAPI:-0}.

> +		;;
> +esac

Or even better, follow standard usage as it can be found in
multilib-minimal or toolchain-funcs:

case ${EAPI} in
	7|8) ;;
	*) die "${ECLASS}: EAPI ${EAPI:-0} not supported" ;;
esac

> +
> +inherit edo
> +
> +

No double empty lines please. (Pkgcheck should have complained about
this.)

> +# @ECLASS_VARIABLE: ROCM_VERSION
> +# @DEFAULT_UNSET
> +# @PRE_INHERIT
> +# @DESCRIPTION:
> +# The ROCm version of current package. Default is ${PV}, but for other packages
> +# that depend on ROCm libraries, this can be set to match the version of
> +# required ROCm libraries.
> +
> +# @ECLASS_VARIABLE: ALL_AMDGPU_TARGETS
> +# @INTERNAL
> +# @DESCRIPTION:
> +# The list of USE flags corresponding to all AMDGPU targets in this ROCm
> +# version. The value depends on ${PV}. Architectures and devices map:
> +# https://www.coelacanth-dream.com/posts/2019/12/30/did-rid-product-matome-p2
> +
> +# @ECLASS_VARIABLE: OFFICIAL_AMDGPU_TARGETS
> +# @INTERNAL
> +# @DESCRIPTION:
> +# The list of USE flags corresponding to all officlially supported AMDGPU

"officially"

> +# targets in this ROCm version, documented at
> +# https://docs.amd.com/bundle/ROCm-Installation-Guide-v${PV}/page/Prerequisite_Actions.html.
> +# USE flag of these architectures will be default on. Depends on ${PV}.
> +
> +# @ECLASS_VARIABLE: ROCM_REQUIRED_USE
> +# @OUTPUT_VARIABLE
> +# @DESCRIPTION:
> +# Requires at least one AMDGPU target to be compiled.
> +# Example use for ROCm libraries:
> +# REQUIRED_USE="${ROCM_REQUIRED_USE}"
> +# Example use for packages that depend on ROCm libraries
> +# IUSE="rocm"
> +# REQUIRED_USE="rocm? ( ${ROCM_REQUIRED_USE} )"
> +
> +# @ECLASS_VARIABLE: ROCM_USEDEP
> +# @OUTPUT_VARIABLE
> +# @DESCRIPTION:
> +# This is an eclass-generated USE-dependency string which can be used to
> +# depend on another ROCm package being built for the same AMDGPU architecture.
> +#
> +# The generated USE-flag list is compatible with packages using rocm.eclass.
> +#
> +# Example use:
> +# @CODE
> +# DEPEND="sci-libs/rocBLAS[${ROCM_USEDEP}]"
> +# @CODE
> +
> +# @FUNCTION: _rocm_set_globals
> +# @DESCRIPTION:
> +# Set global variables used by the eclass: ALL_AMDGPU_TARGETS,
> +# OFFICIAL_AMDGPU_TARGETS, ROCM_REQUIRED_USE, and ROCM_USEDEP
> +_rocm_set_globals() {
> +	case ${ROCM_VERSION:-${PV}} in
> +		4*)
> +			ALL_AMDGPU_TARGETS=(
> +				gfx803 gfx900 gfx906 gfx908
> +				gfx1010 gfx1011 gfx1012 gfx1030
> +			)
> +			OFFICIAL_AMDGPU_TARGETS=(
> +				gfx906 gfx908
> +			)
> +			;;
> +		5*)
> +			ALL_AMDGPU_TARGETS=(
> +				gfx803 gfx900 gfx906 gfx908 gfx90a
> +				gfx1010 gfx1011 gfx1012 gfx1030 gfx1031
> +			)
> +			OFFICIAL_AMDGPU_TARGETS=(
> +				gfx906 gfx908 gfx90a gfx1030
> +			)
> +			;;
> +		*)
> +			die "Unknown ROCm major version! Please update rocm.eclass before bumping to new ebuilds"
> +			;;
> +	esac
> +
> +	ROCM_REQUIRED_USE+=" || ("
> +	for gpu_target in ${ALL_AMDGPU_TARGETS[@]}; do

Add a pair of double quotes here.

> +		if [[ " ${OFFICIAL_AMDGPU_TARGETS[*]} " =~ " ${gpu_target} " ]]; then

So OFFICIAL_AMDGPU_TARGETS is an array, but then it's flattened to a
string for the test? Either use "has" for the test, or don't use an
array in the first place.

> +			IUSE+=" ${gpu_target/#/+amdgpu_targets_}"

Why "+="? I don't see any previous assignment of IUSE.

> +		else
> +			IUSE+=" ${gpu_target/#/amdgpu_targets_}"

Ditto.

> +		fi
> +		ROCM_REQUIRED_USE+=" ${gpu_target/#/amdgpu_targets_}"
> +	done
> +	ROCM_REQUIRED_USE+=" ) "
> +
> +	local flags=( "${ALL_AMDGPU_TARGETS[@]/#/amdgpu_targets_}" )
> +	local optflags=${flags[@]/%/(-)?}
> +	ROCM_USEDEP=${optflags// /,}
> +}
> +_rocm_set_globals
> +unset -f _rocm_set_globals
> +
> +
> +# @FUNCTION: get_amdgpu_flags
> +# @USAGE: get_amdgpu_flags
> +# @DESCRIPTION:
> +# Convert specified use flag of amdgpu_targets to compilation flags.
> +# Append default target feature to GPU arch. See
> +# https://llvm.org/docs/AMDGPUUsage.html#target-features
> +get_amdgpu_flags() {
> +	local AMDGPU_TARGET_FLAGS
> +	for gpu_target in ${ALL_AMDGPU_TARGETS[@]}; do

Double quotes please.

> +		local target_feature=
> +		if use amdgpu_targets_${gpu_target}; then
> +			case ${gpu_target} in
> +				gfx906|gfx908)
> +					target_feature=:xnack-
> +					;;
> +				gfx90a)
> +					target_feature=:xnack+
> +					;;
> +				*)
> +					;;
> +			esac
> +			AMDGPU_TARGET_FLAGS+="${gpu_target}${target_feature};"
> +		fi
> +	done
> +	echo ${AMDGPU_TARGET_FLAGS}
> +}
> +
> +# @FUNCTION: check_rw_permission
> +# @USAGE: check_rw_permission <file>
> +# @DESCRIPTION:
> +# check read and write permissions on specific files.
> +# allow using wildcard, for example check_rw_permission /dev/dri/render*
> +check_rw_permission() {
> +	[[ -r "$1" ]] && [[ -w "$1" ]] || die \

Quotation marks aren't necessary here.

> +		"${PORTAGE_USERNAME} do not have read or write permissions on $1! \n Make sure ${PORTAGE_USERNAME} is in render group and check the permissions."

Please don't use internal Portage variables.

> +}
> +
> +# == phase functions ==
> +
> +# @FUNCTION: rocm_src_configure
> +# @DESCRIPTION:
> +# configure rocm packages, and setting common cmake arguments
> +rocm_src_configure() {
> +	# allow acces to hardware
> +	addpredict /dev/kfd
> +	addpredict /dev/dri/
> +
> +	mycmakeargs+=(
> +		-DCMAKE_INSTALL_PREFIX="${EPREFIX}/usr"
> +		-DAMDGPU_TARGETS="$(get_amdgpu_flags)"
> +		-DCMAKE_SKIP_RPATH=TRUE
> +	)
> +
> +	CXX="hipcc" cmake_src_configure
> +}
> +
> +# @FUNCTION: rocm_src_test
> +# @DESCRIPTION:
> +# Test whether valid GPU device is present. If so, find how to, and execute test.
> +# ROCm packages can have to test mechanism:
> +# 1. cmake_src_test. Set MAKEOPTS="-j1" to make sure only one test on GPU at a time;
> +# 2. one single gtest binary called "${PN,,}"-test;
> +# 3. Some package like rocFFT have alternative test like rocfft-selftest;
> +# 4. Custome testing binaries like dev-libs/rccl. Use ${ROCM_TESTS} to specify.
> +rocm_src_test() {
> +	addwrite /dev/kfd
> +	addwrite /dev/dri/
> +
> +	# check permissions on /dev/kfd and /dev/dri/render*
> +	check_rw_permission /dev/kfd
> +	check_rw_permission /dev/dri/render*
> +
> +	: ${LD_LIBRARY_PATH:="${BUILD_DIR}/clients:${BUILD_DIR}/src:${BUILD_DIR}/library:${BUILD_DIR}/library/src:${BUILD_DIR}/library/src/device"}
> +	export LD_LIBRARY_PATH
> +	if grep -q 'build test:' "${BUILD_DIR}"/build.ninja; then
> +		MAKEOPTS="-j1" cmake_src_test
> +	elif [[ -d "${BUILD_DIR}"/clients/staging ]]; then

Quotation marks aren't necessary here.

> +		cd "${BUILD_DIR}/clients/staging" || die "Test directory not found!"
> +		for test_program in "${PN,,}-"*test; do
> +			if [[ -x ${test_program} ]]; then
> +					edob ./${test_program}
> +			else
> +				die "The test program ${test_program} does not exist or cannot be excuted!"
> +			fi
> +		done
> +	elif [[ ! -z "${ROCM_TESTS}" ]]; then

Again, no quotation marks.

Also, why the double negation? IMHO "-n" would be better readable.

> +		for test_program in "${ROCM_TESTS}"; do

What is this supposed to do? The loop will be executed exactly once,
whatever ROCM_TESTS contains.

> +			cd "${BUILD_DIR}" || die
> +			if [[ -x ${test_program} ]]; then
> +			edob ./${test_program}
> +			else
> +				die "The test program ${test_program} does not exist or cannot be excuted!"
> +			fi
> +		done
> +	else
> +		die "There is no cmake tests, no \${ROCM_TESTS} executable provided, nor ${BUILD_DIR}/clients/staging where test program might be located."
> +	fi
> +}
> +
> +_ROCM_ECLASS=1
> +fi