On Mon, Nov 23, 2020 at 10:00 AM Martin Liška <mli...@suse.cz> wrote:
>
> On 11/20/20 10:49 AM, Richard Biener wrote:
> > On Fri, Apr 3, 2020 at 8:15 PM Egeyar Bagcioglu
> > <egeyar.bagcio...@oracle.com> wrote:
> >>
> >>
> >>
> >> On 3/18/20 10:05 AM, Martin Liška wrote:
> >>> On 3/17/20 7:43 PM, Egeyar Bagcioglu wrote:
> >>>> Hi Martin,
> >>>>
> >>>> I like the patch. It definitely serves our purposes at Oracle and
> >>>> provides another way to do what my previous patches did as well.
> >>>>
> >>>> 1) It keeps the backwards compatibility regarding
> >>>> -frecord-gcc-switches; therefore, removes my related doubts about
> >>>> your previous patch.
> >>>>
> >>>> 2) It still makes use of -frecord-gcc-switches. The new option is
> >>>> only to control the format. This addresses some previous objections
> >>>> to having a new option doing something similar. Now the new option
> >>>> controls the behaviour of the existing one and that behaviour can be
> >>>> further extended.
> >>>>
> >>>> 3) It uses an environment variable as Jakub suggested.
> >>>>
> >>>> The patch looks good and I confirm that it works for our purposes.
> >>>
> >>> Hello.
> >>>
> >>> Thank you for the support.
> >>>
> >>>>
> >>>> Having said that, I have to ask for recognition in this patch for my
> >>>> and my company's contributions. Can you please keep my name and my
> >>>> work email in the changelog and in the commit message?
> >>>
> >>> Sure, sorry I forgot.
> >>
> >> Hi Martin,
> >>
> >> I noticed that some comments in the patch were still referring to
> >> --record-gcc-command-line, the option I suggested earlier. I updated
> >> those comments to mention -frecord-gcc-switches-format instead and also
> >> added my name to the patch as you agreed above. I attached the updated
> >> patch. We are starting to use this patch in the specific domain where we
> >> need its functionality.
> >
> > So while I like the addition of -frecord-gcc-switches-format to preserve
> > backward compatibility there are IMHO several issues with the patch.
>
> Hey.
>
> >
> > For one, the target hook change to a void(void) hook makes it need to
> > duplicate too much internals. Please make it take a const char *
> > argument, the actual text to output.
>
> Agree with that, nice improvement.
>
> >
> > For second, I see no good reason to have different handling of
> > -grecord-gcc-switches vs. -frecord-gcc-switches - they should
> > produce the same content and thus -frecord-gcc-switches-format
> > should apply to -grecord-gcc-switches as well.
>
> They do. Both respect the newly added -frecord-gcc-switches-format argument.
>
> >
> > Now - I think what we output into the assembly file with -fverbose-asm
> > should _always_ be the processed arguments since the assembly
> > is produced by cc1, not gcc.
>
> All right, changed that.
>
> >
> > +/* Return value of env variable GCC_DRIVER_COMMAND_LINE if exists.
> > + Otherwise return empty string. */
> > +
> > +const char *
> > +get_driver_command_line ()
> > +{
> > + const char *cmdline = getenv ("GCC_DRIVER_COMMAND_LINE");
> > + return cmdline != NULL ? cmdline : "";
> > +}
> >
> > I think silently emitting nothing is not a good idea. In particular using
> > the environment to carry the driver command-line makes it difficult
> > to reproduce output with pasting commands as dumped by -v [-save-temps].
> > Likewise the driver seems to always populate GCC_DRIVER_COMMAND_LINE
> > even if not needed - why not look for -frecord-gcc-switches-format before
> > doing so? I'd make -frecord-gcc-switches-format a driver only option
> > (only the driver can do sth about it) and amend -frecord-gcc-switches
> > with a -frecord-gcc-switches=FILE variant specifying the content. The
> > driver would then substitute -frecord-gcc-switches-format=driver
> > with -frecord-gcc-switches=tempfile and dump the command line into
> > tempfile. cc1 can then pick contents from tempfile or use the processed
> > variant if no contents are specified.
>
> I've just rewritten that to -frecord-gcc-switches-file option. Note that
> we can't transform that to -frecord-gcc-switches=FILE as one can use:
>
> -g frecord-gcc-switches-format=driver and so -frecord-gcc-switches is off.
But the driver can check whether -frecord-gcc-switches is on with
sth like
%{frecord-gcc-switches:%{frecord-gcc-switches-format=driver:-frecord-gcc-switches=%b}}
or so. But I don't mind a separate -frecord-gcc-switches-file option.
Guess we need it anyway since -g implies -grecord-gcc-switchs and that
needs the info as well.
> Thoughts on the new version of the patch?
Can you split out the unifying of -[gf]record-gcc-switches processing
and the target hook adjustment from the change introducing
-frecord-gcc-switches-format?
dwarf2out.c seems to retain its gen_producer_string () even though
you duplicate it elsewhere and it is now unused? Please retain
the gen_producer_string name since the function does actual
processing and not just fetch a precomputed string from somewhere.
I'd like somebody else chime in on the -frecord-gcc-switches-format
driver handling but will happily work with getting the unification part merged.
Richard.
> Thanks,
> Martin
>
> >
> > Richard.
> >
> >
> >> Regards
> >> Egeyar
> >>
> >>
> >>>
> >>> Martin
> >>>
> >>>>
> >>>> Thanks
> >>>> Egeyar
> >>>>
> >>>>
> >>>>
> >>>> On 3/17/20 2:53 PM, Martin Liška wrote:
> >>>>> Hi.
> >>>>>
> >>>>> I'm sending enhanced patch that makes the following changes:
> >>>>> - a new option -frecord-gcc-switches-format is added; the option
> >>>>> selects format (processed, driver) for all options that record
> >>>>> GCC command line
> >>>>> - Dwarf gen_produce_string is now used in -fverbose-asm
> >>>>> - The .s file is affected in the following way:
> >>>>>
> >>>>> BEFORE:
> >>>>>
> >>>>> # GNU C17 (SUSE Linux) version 9.2.1 20200128 [revision
> >>>>> 83f65674e78d97d27537361de1a9d74067ff228d] (x86_64-suse-linux)
> >>>>> # compiled by GNU C version 9.2.1 20200128 [revision
> >>>>> 83f65674e78d97d27537361de1a9d74067ff228d], GMP version 6.2.0, MPFR
> >>>>> version 4.0.2, MPC version 1.1.0, isl version isl-0.22.1-GMP
> >>>>>
> >>>>> # GGC heuristics: --param ggc-min-expand=100 --param
> >>>>> ggc-min-heapsize=131072
> >>>>> # options passed: -fpreprocessed test.i -march=znver1 -mmmx -mno-3dnow
> >>>>> # -msse -msse2 -msse3 -mssse3 -msse4a -mcx16 -msahf -mmovbe -maes -msha
> >>>>> # -mpclmul -mpopcnt -mabm -mno-lwp -mfma -mno-fma4 -mno-xop -mbmi
> >>>>> -mno-sgx
> >>>>> # -mbmi2 -mno-pconfig -mno-wbnoinvd -mno-tbm -mavx -mavx2 -msse4.2
> >>>>> -msse4.1
> >>>>> # -mlzcnt -mno-rtm -mno-hle -mrdrnd -mf16c -mfsgsbase -mrdseed -mprfchw
> >>>>> # -madx -mfxsr -mxsave -mxsaveopt -mno-avx512f -mno-avx512er
> >>>>> -mno-avx512cd
> >>>>> # -mno-avx512pf -mno-prefetchwt1 -mclflushopt -mxsavec -mxsaves
> >>>>> # -mno-avx512dq -mno-avx512bw -mno-avx512vl -mno-avx512ifma
> >>>>> -mno-avx512vbmi
> >>>>> # -mno-avx5124fmaps -mno-avx5124vnniw -mno-clwb -mmwaitx -mclzero
> >>>>> -mno-pku
> >>>>> # -mno-rdpid -mno-gfni -mno-shstk -mno-avx512vbmi2 -mno-avx512vnni
> >>>>> # -mno-vaes -mno-vpclmulqdq -mno-avx512bitalg -mno-movdiri
> >>>>> -mno-movdir64b
> >>>>> # -mno-waitpkg -mno-cldemote -mno-ptwrite --param l1-cache-size=32
> >>>>> # --param l1-cache-line-size=64 --param l2-cache-size=512 -mtune=znver1
> >>>>> # -grecord-gcc-switches -g -fverbose-asm -frecord-gcc-switches
> >>>>> # options enabled: -faggressive-loop-optimizations -fassume-phsa
> >>>>> # -fasynchronous-unwind-tables -fauto-inc-dec -fcommon
> >>>>> # -fdelete-null-pointer-checks -fdwarf2-cfi-asm -fearly-inlining
> >>>>> # -feliminate-unused-debug-types -ffp-int-builtin-inexact
> >>>>> -ffunction-cse
> >>>>> # -fgcse-lm -fgnu-runtime -fgnu-unique -fident -finline-atomics
> >>>>> # -fipa-stack-alignment -fira-hoist-pressure -fira-share-save-slots
> >>>>> # -fira-share-spill-slots -fivopts -fkeep-static-consts
> >>>>> # -fleading-underscore -flifetime-dse -flto-odr-type-merging
> >>>>> -fmath-errno
> >>>>> # -fmerge-debug-strings -fpeephole -fplt -fprefetch-loop-arrays
> >>>>> # -frecord-gcc-switches -freg-struct-return
> >>>>> -fsched-critical-path-heuristic
> >>>>> # -fsched-dep-count-heuristic -fsched-group-heuristic
> >>>>> -fsched-interblock
> >>>>> # -fsched-last-insn-heuristic -fsched-rank-heuristic -fsched-spec
> >>>>> # -fsched-spec-insn-heuristic -fsched-stalled-insns-dep
> >>>>> -fschedule-fusion
> >>>>> # -fsemantic-interposition -fshow-column -fshrink-wrap-separate
> >>>>> # -fsigned-zeros -fsplit-ivs-in-unroller -fssa-backprop -fstdarg-opt
> >>>>> # -fstrict-volatile-bitfields -fsync-libcalls -ftrapping-math
> >>>>> -ftree-cselim
> >>>>> # -ftree-forwprop -ftree-loop-if-convert -ftree-loop-im
> >>>>> -ftree-loop-ivcanon
> >>>>> # -ftree-loop-optimize -ftree-parallelize-loops= -ftree-phiprop
> >>>>> # -ftree-reassoc -ftree-scev-cprop -funit-at-a-time -funwind-tables
> >>>>> # -fverbose-asm -fzero-initialized-in-bss -m128bit-long-double -m64
> >>>>> -m80387
> >>>>> # -mabm -madx -maes -malign-stringops -mavx -mavx2
> >>>>> # -mavx256-split-unaligned-store -mbmi -mbmi2 -mclflushopt -mclzero
> >>>>> -mcx16
> >>>>> # -mf16c -mfancy-math-387 -mfma -mfp-ret-in-387 -mfsgsbase -mfxsr
> >>>>> -mglibc
> >>>>> # -mieee-fp -mlong-double-80 -mlzcnt -mmmx -mmovbe -mmwaitx -mpclmul
> >>>>> # -mpopcnt -mprfchw -mpush-args -mrdrnd -mrdseed -mred-zone -msahf
> >>>>> -msha
> >>>>> # -msse -msse2 -msse3 -msse4 -msse4.1 -msse4.2 -msse4a -mssse3 -mstv
> >>>>> # -mtls-direct-seg-refs -mvzeroupper -mxsave -mxsavec -mxsaveopt
> >>>>> -mxsaves
> >>>>>
> >>>>> AFTER:
> >>>>>
> >>>>> # GGC heuristics: --param ggc-min-expand=30 --param
> >>>>> ggc-min-heapsize=4096
> >>>>> # GNU C17 10.0.1 20200317 (experimental) -march=znver1 -mmmx
> >>>>> -mno-3dnow -msse -msse2 -msse3 -mssse3 -msse4a -mcx16 -msahf -mmovbe
> >>>>> -maes -msha -mpclmul -mpopcnt -mabm -mno-lwp -mfma -mno-fma4
> >>>>> -mno-xop -mbmi -mno-sgx -mbmi2 -mno-pconfig -mno-wbnoinvd -mno-tbm
> >>>>> -mavx -mavx2 -msse4.2 -msse4.1 -mlzcnt -mno-rtm -mno-hle -mrdrnd
> >>>>> -mf16c -mfsgsbase -mrdseed -mprfchw -madx -mfxsr -mxsave -mxsaveopt
> >>>>> -mno-avx512f -mno-avx512er -mno-avx512cd -mno-avx512pf
> >>>>> -mno-prefetchwt1 -mclflushopt -mxsavec -mxsaves -mno-avx512dq
> >>>>> -mno-avx512bw -mno-avx512vl -mno-avx512ifma -mno-avx512vbmi
> >>>>> -mno-avx5124fmaps -mno-avx5124vnniw -mno-clwb -mmwaitx -mclzero
> >>>>> -mno-pku -mno-rdpid -mno-gfni -mno-shstk -mno-avx512vbmi2
> >>>>> -mno-avx512vnni -mno-vaes -mno-vpclmulqdq -mno-avx512bitalg
> >>>>> -mno-movdiri -mno-movdir64b -mno-waitpkg -mno-cldemote -mno-ptwrite
> >>>>> -mno-avx512bf16 -mno-enqcmd -mno-avx512vp2intersect
> >>>>> --param=l1-cache-size=32 --param=l1-cache-line-size=64
> >>>>> --param=l2-cache-size=512 -mtune=znver1 -g
> >>>>>
> >>>>> That's the biggest change I made, but I hope it's acceptable.
> >>>>> Apart from that the patch simplifies and unifies places where
> >>>>> save_decoded_options
> >>>>> options are transformed back to string representation.
> >>>>>
> >>>>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> >>>>>
> >>>>> Ready to be installed in next stage1?
> >>>>> Thanks,
> >>>>> Martin
> >>>>
> >>>
> >>
>
