On Tue, May 20, 2025 at 3:09 AM Kugan Vivekanandarajah <kvivekana...@nvidia.com> wrote: > > 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.
> * Makefile.def: AUTO_PROFILE based on cpu_type. > * Makefile.in: Likewise. Makefile.in is a generated file (from Makefile.def and Makefile.tpl), It looks like you edited the file instead of regenerated it. Can you please regenerate the file and/or provide the corresponding corrected changes to Makefile.def/Makefile.tpl which was used to regenerate Makefile.in? This is what https://gcc.gnu.org/pipermail/gcc-testresults/2025-May/848013.html is about too. Thanks, Andrew Pinski > > Thanks, > Kugan > > > > > > > Thanks, > > Richard > > > >> +fi >
