> On 26 May 2025, at 2:25 pm, Andrew Pinski <pins...@gmail.com> wrote: > > External email: Use caution opening links or attachments > > > 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.
Apologies for the breakage. Attached patch fixes this, Is this OK? Thanks, Kugan > > Thanks, > Andrew Pinski > > >> >> Thanks, >> Kugan >> >> >> >>> >>> Thanks, >>> Richard >>> >>>> +fi
0001-AUTOFDO-Fix-autogen-remake-issue.patch
Description: 0001-AUTOFDO-Fix-autogen-remake-issue.patch
