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.
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.
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.
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.
+/* 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.
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
> >>
> >
>
