> On 26 May 2025, at 2:47 pm, Kugan Vivekanandarajah <kvivekana...@nvidia.com> > wrote: > > External email: Use caution opening links or attachments > > > > 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?
This patch is effectively is the same as my earlier approved patch that is committed. It just moves the changes to Makefile.tpl such that it is generated. I think this is obvious and want to commit soon to get the build tests passing again. I will commit unless there is any objection. Thanks, Kugan > > > Thanks, > Kugan > > > > > > > Thanks, > > Andrew Pinski > > > > > >> > >> Thanks, > >> Kugan > >> > >> > >> > >>> > >>> Thanks, > >>> Richard > >>> > >>>> +fi > > > <0001-AUTOFDO-Fix-autogen-remake-issue.patch>
