Thanks Richard for the review. > On 20 May 2025, at 2:47 am, Richard Sandiford <richard.sandif...@arm.com> > wrote: > > External email: Use caution opening links or attachments > > > Kugan Vivekanandarajah <kvivekana...@nvidia.com> writes: >> diff --git a/Makefile.in b/Makefile.in >> index b1ed67d3d4f..b5e3e520791 100644 >> --- a/Makefile.in >> +++ b/Makefile.in >> @@ -4271,7 +4271,7 @@ all-stageautoprofile-bfd: >> configure-stageautoprofile-bfd >> $(HOST_EXPORTS) \ >> $(POSTSTAGE1_HOST_EXPORTS) \ >> cd $(HOST_SUBDIR)/bfd && \ >> - $$s/gcc/config/i386/$(AUTO_PROFILE) \ >> + $$s/gcc/config/@cpu_type@/$(AUTO_PROFILE) \ >> $(MAKE) $(BASE_FLAGS_TO_PASS) \ >> CFLAGS="$(STAGEautoprofile_CFLAGS)" \ >> GENERATOR_CFLAGS="$(STAGEautoprofile_GENERATOR_CFLAGS)" \ > > The usual style seems to be to assign @foo@ to a makefile variable > called foo or FOO, rather than to use @foo@ directly in rules. Otherwise > the makefile stuff looks good. > > I don't feel qualified to review the script, but some general shell stuff: > >> diff --git a/gcc/config/aarch64/gcc-auto-profile >> b/gcc/config/aarch64/gcc-auto-profile >> new file mode 100755 >> index 00000000000..0ceec035e69 >> --- /dev/null >> +++ b/gcc/config/aarch64/gcc-auto-profile >> @@ -0,0 +1,51 @@ >> +#!/bin/sh >> +# Profile workload for gcc profile feedback (autofdo) using Linux perf. >> +# Copyright The GNU Toolchain Authors. >> +# >> +# This file is part of GCC. >> +# >> +# GCC is free software; you can redistribute it and/or modify it under >> +# the terms of the GNU General Public License as published by the Free >> +# Software Foundation; either version 3, or (at your option) any later >> +# version. >> + >> +# GCC is distributed in the hope that it will be useful, but WITHOUT ANY >> +# WARRANTY; without even the implied warranty of MERCHANTABILITY or >> +# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License >> +# for more details. >> + >> +# You should have received a copy of the GNU General Public License >> +# along with GCC; see the file COPYING3. If not see >> +# <http://www.gnu.org/licenses/>. */ >> + >> +# Run perf record with branch stack sampling and check for >> +# specific error message to see if it is supported. >> +use_brbe=true >> +output=$(perf record -j any,u ls 2>&1) > > How about using /bin/true rather than ls for the test program? > >> +if [[ "$output" = *"Error::P: PMU Hardware or event type doesn't support >> branch stack sampling."* ]]; then > > [[ isn't POSIX, or at least dash doesn't accept it. Since this script > is effectively linux-specific, we can probably assume that /bin/bash > exists and use that in the #! line. > > If we use bash, then the test could use =~ rather than an exact match. > This could be useful if perf prints other diagnostics besides the > one being tested for, or if future versions of perf alter the wording > slightly. > >> + use_brbe=false >> +fi >> + >> +FLAGS=u >> +if [ "$1" = "--kernel" ] ; then >> + FLAGS=k >> + shift >> +fi >> +if [ "$1" = "--all" ] ; then > > How about making this an elif, so that we don't accept --kernel --all? > >> + FLAGS=u,k >> + shift >> +fi >> + >> +if [ "$use_brbe" = true ] ; then >> + if grep -q hypervisor /proc/cpuinfo ; then >> + echo >&2 "Warning: branch profiling may not be functional in VMs" >> + fi >> + set -x >> + perf record -j any,$FLAGS "$@" >> + set +x >> +else >> + set -x >> + echo >&2 "Warning: branch profiling may not be functional without BRBE" >> + perf record "$@" >> + set +x > > Putting the set -x after the echo seems better, as for the "then" branch.
Here is the revised version that handles the above comments. Thanks, Kugan > > Thanks, > Richard > >> +fi
0004-AUTOFDO_v3-AARCH64-Add-support-for-profilebootstrap.patch
Description: 0004-AUTOFDO_v3-AARCH64-Add-support-for-profilebootstrap.patch
