| Message ID | 20221007203236.1750890-1-ndesaulniers@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kbuild: add -fno-discard-value-names to cmd_cc_ll_c | expand |
On Fri, Oct 07, 2022 at 01:32:36PM -0700, Nick Desaulniers wrote: > When debugging LLVM IR, it can be handy for clang to not discard value > names used for local variables and parameters. Compare the generated IR. > > Implicit Default (-fdiscard-value-names): > define i32 @core_sys_select(i32 %0, ptr %1, ptr %2, ptr %3, ptr %4) { > %6 = alloca i64 > %7 = alloca %struct.poll_wqueues > %8 = alloca [64 x i32] > > Explicit -fno-discard-value-names: > define i32 @core_sys_select(i32 %n, ptr %inp, ptr %outp, ptr %exp, > ptr %end_time) { > %expire.i = alloca i64 > %table.i = alloca %struct.poll_wqueues > %stack_fds = alloca [64 x i32] > > The rule for generating human readable LLVM IR (.ll) is only useful as a > debugging feature: > > $ make LLVM=1 fs/select.ll > > Clang defaults to -fdiscard-value-names to save memory when generating > LLVM IR. For debugging purposes, the improvement in readability at a > cost of more verbose IR is a cost we're happy to pay. > > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> Thanks for the patch! Link: https://github.com/ClangBuiltLinux/linux/issues/1467 Reviewed-by: Nathan Chancellor <nathan@kernel.org> > --- > scripts/Makefile.build | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/scripts/Makefile.build b/scripts/Makefile.build > index 27be77c0d6d8..d0e4f476dfee 100644 > --- a/scripts/Makefile.build > +++ b/scripts/Makefile.build > @@ -140,7 +140,7 @@ $(obj)/%.symtypes : $(src)/%.c FORCE > # LLVM assembly > # Generate .ll files from .c > quiet_cmd_cc_ll_c = CC $(quiet_modtag) $@ > - cmd_cc_ll_c = $(CC) $(c_flags) -emit-llvm -S -o $@ $< > + cmd_cc_ll_c = $(CC) $(c_flags) -emit-llvm -S -fno-discard-value-names -o $@ $< > > $(obj)/%.ll: $(src)/%.c FORCE > $(call if_changed_dep,cc_ll_c) > > base-commit: 93ed07a23fd08b8613f64cf0a15d7fbdaca010fd > -- > 2.38.0.rc2.412.g84df46c1b4-goog >
On 2022-10-13, Nathan Chancellor wrote: >On Fri, Oct 07, 2022 at 01:32:36PM -0700, Nick Desaulniers wrote: >> When debugging LLVM IR, it can be handy for clang to not discard value >> names used for local variables and parameters. Compare the generated IR. >> >> Implicit Default (-fdiscard-value-names): >> define i32 @core_sys_select(i32 %0, ptr %1, ptr %2, ptr %3, ptr %4) { >> %6 = alloca i64 >> %7 = alloca %struct.poll_wqueues >> %8 = alloca [64 x i32] >> >> Explicit -fno-discard-value-names: >> define i32 @core_sys_select(i32 %n, ptr %inp, ptr %outp, ptr %exp, >> ptr %end_time) { >> %expire.i = alloca i64 >> %table.i = alloca %struct.poll_wqueues >> %stack_fds = alloca [64 x i32] >> >> The rule for generating human readable LLVM IR (.ll) is only useful as a >> debugging feature: >> >> $ make LLVM=1 fs/select.ll >> >> Clang defaults to -fdiscard-value-names to save memory when generating >> LLVM IR. For debugging purposes, the improvement in readability at a >> cost of more verbose IR is a cost we're happy to pay. A LLVM_ENABLE_ASSEERTIONS=off build of Clang defaults to -fdiscard-value-names. A LLVM_ENABLE_ASSEERTIONS=on build of Clang defaults to -fno-discard-value-names. See clang/lib/Driver/ToolChains/Clang.cpp:5030 >> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> > >Thanks for the patch! > >Link: https://github.com/ClangBuiltLinux/linux/issues/1467 >Reviewed-by: Nathan Chancellor <nathan@kernel.org> Reviewed-by: Fangrui Song <maskray@google.com> >> --- >> scripts/Makefile.build | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/scripts/Makefile.build b/scripts/Makefile.build >> index 27be77c0d6d8..d0e4f476dfee 100644 >> --- a/scripts/Makefile.build >> +++ b/scripts/Makefile.build >> @@ -140,7 +140,7 @@ $(obj)/%.symtypes : $(src)/%.c FORCE >> # LLVM assembly >> # Generate .ll files from .c >> quiet_cmd_cc_ll_c = CC $(quiet_modtag) $@ >> - cmd_cc_ll_c = $(CC) $(c_flags) -emit-llvm -S -o $@ $< >> + cmd_cc_ll_c = $(CC) $(c_flags) -emit-llvm -S -fno-discard-value-names -o $@ $< >> >> $(obj)/%.ll: $(src)/%.c FORCE >> $(call if_changed_dep,cc_ll_c) >> >> base-commit: 93ed07a23fd08b8613f64cf0a15d7fbdaca010fd >> -- >> 2.38.0.rc2.412.g84df46c1b4-goog >> >
On Thu, Oct 13, 2022 at 1:41 PM Fangrui Song <maskray@google.com> wrote: > > On 2022-10-13, Nathan Chancellor wrote: > >On Fri, Oct 07, 2022 at 01:32:36PM -0700, Nick Desaulniers wrote: > >> When debugging LLVM IR, it can be handy for clang to not discard value > >> names used for local variables and parameters. Compare the generated IR. > >> > >> Implicit Default (-fdiscard-value-names): > >> define i32 @core_sys_select(i32 %0, ptr %1, ptr %2, ptr %3, ptr %4) { > >> %6 = alloca i64 > >> %7 = alloca %struct.poll_wqueues > >> %8 = alloca [64 x i32] > >> > >> Explicit -fno-discard-value-names: > >> define i32 @core_sys_select(i32 %n, ptr %inp, ptr %outp, ptr %exp, > >> ptr %end_time) { > >> %expire.i = alloca i64 > >> %table.i = alloca %struct.poll_wqueues > >> %stack_fds = alloca [64 x i32] > >> > >> The rule for generating human readable LLVM IR (.ll) is only useful as a > >> debugging feature: > >> > >> $ make LLVM=1 fs/select.ll > >> > >> Clang defaults to -fdiscard-value-names to save memory when generating > >> LLVM IR. For debugging purposes, the improvement in readability at a > >> cost of more verbose IR is a cost we're happy to pay. > > A LLVM_ENABLE_ASSEERTIONS=off build of Clang defaults to -fdiscard-value-names. > > A LLVM_ENABLE_ASSEERTIONS=on build of Clang defaults to -fno-discard-value-names. > > See clang/lib/Driver/ToolChains/Clang.cpp:5030 Thanks! I thought I recalled this; I was literally just rereading through https://github.com/ClangBuiltLinux/linux/issues/1467#issuecomment-1046071624 again to understand. That would be useful to include in the commit message, and is more precise than "Clang defaults to -fdiscard-value-names to save memory when generating LLVM IR." Masahiro, would you mind including or replacing that, or shall I send a v2? I was surprised to find later in the day that I was running an assertions-disabled (ie. release) build of clang, which I almost never do! Non-llvm-developers are more likely to be using release builds than assertions-enabled builds of llvm, so this patch still very much has merit. > > >> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> > > > >Thanks for the patch! > > > >Link: https://github.com/ClangBuiltLinux/linux/issues/1467 > >Reviewed-by: Nathan Chancellor <nathan@kernel.org> > > Reviewed-by: Fangrui Song <maskray@google.com> > > >> --- > >> scripts/Makefile.build | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/scripts/Makefile.build b/scripts/Makefile.build > >> index 27be77c0d6d8..d0e4f476dfee 100644 > >> --- a/scripts/Makefile.build > >> +++ b/scripts/Makefile.build > >> @@ -140,7 +140,7 @@ $(obj)/%.symtypes : $(src)/%.c FORCE > >> # LLVM assembly > >> # Generate .ll files from .c > >> quiet_cmd_cc_ll_c = CC $(quiet_modtag) $@ > >> - cmd_cc_ll_c = $(CC) $(c_flags) -emit-llvm -S -o $@ $< > >> + cmd_cc_ll_c = $(CC) $(c_flags) -emit-llvm -S -fno-discard-value-names -o $@ $< > >> > >> $(obj)/%.ll: $(src)/%.c FORCE > >> $(call if_changed_dep,cc_ll_c) > >> > >> base-commit: 93ed07a23fd08b8613f64cf0a15d7fbdaca010fd > >> -- > >> 2.38.0.rc2.412.g84df46c1b4-goog > >> > >
On Fri, Oct 14, 2022 at 5:54 AM Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Thu, Oct 13, 2022 at 1:41 PM Fangrui Song <maskray@google.com> wrote: > > > > On 2022-10-13, Nathan Chancellor wrote: > > >On Fri, Oct 07, 2022 at 01:32:36PM -0700, Nick Desaulniers wrote: > > >> When debugging LLVM IR, it can be handy for clang to not discard value > > >> names used for local variables and parameters. Compare the generated IR. > > >> > > >> Implicit Default (-fdiscard-value-names): > > >> define i32 @core_sys_select(i32 %0, ptr %1, ptr %2, ptr %3, ptr %4) { > > >> %6 = alloca i64 > > >> %7 = alloca %struct.poll_wqueues > > >> %8 = alloca [64 x i32] > > >> > > >> Explicit -fno-discard-value-names: > > >> define i32 @core_sys_select(i32 %n, ptr %inp, ptr %outp, ptr %exp, > > >> ptr %end_time) { > > >> %expire.i = alloca i64 > > >> %table.i = alloca %struct.poll_wqueues > > >> %stack_fds = alloca [64 x i32] > > >> > > >> The rule for generating human readable LLVM IR (.ll) is only useful as a > > >> debugging feature: > > >> > > >> $ make LLVM=1 fs/select.ll > > >> > > >> Clang defaults to -fdiscard-value-names to save memory when generating > > >> LLVM IR. For debugging purposes, the improvement in readability at a > > >> cost of more verbose IR is a cost we're happy to pay. > > > > A LLVM_ENABLE_ASSEERTIONS=off build of Clang defaults to -fdiscard-value-names. > > > > A LLVM_ENABLE_ASSEERTIONS=on build of Clang defaults to -fno-discard-value-names. > > > > See clang/lib/Driver/ToolChains/Clang.cpp:5030 > > Thanks! > > I thought I recalled this; I was literally just rereading through > https://github.com/ClangBuiltLinux/linux/issues/1467#issuecomment-1046071624 > again to understand. > > That would be useful to include in the commit message, and is more > precise than "Clang defaults to -fdiscard-value-names to save memory > when generating LLVM IR." Masahiro, would you mind including or > replacing that, or shall I send a v2? Can you send v2, or provide the entire new commit description? Thank you. > > I was surprised to find later in the day that I was running an > assertions-disabled (ie. release) build of clang, which I almost never > do! Non-llvm-developers are more likely to be using release builds > than assertions-enabled builds of llvm, so this patch still very much > has merit. > > > > > >> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> > > > > > >Thanks for the patch! > > > > > >Link: https://github.com/ClangBuiltLinux/linux/issues/1467 > > >Reviewed-by: Nathan Chancellor <nathan@kernel.org> > > > > Reviewed-by: Fangrui Song <maskray@google.com> > > > > >> --- > > >> scripts/Makefile.build | 2 +- > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > >> > > >> diff --git a/scripts/Makefile.build b/scripts/Makefile.build > > >> index 27be77c0d6d8..d0e4f476dfee 100644 > > >> --- a/scripts/Makefile.build > > >> +++ b/scripts/Makefile.build > > >> @@ -140,7 +140,7 @@ $(obj)/%.symtypes : $(src)/%.c FORCE > > >> # LLVM assembly > > >> # Generate .ll files from .c > > >> quiet_cmd_cc_ll_c = CC $(quiet_modtag) $@ > > >> - cmd_cc_ll_c = $(CC) $(c_flags) -emit-llvm -S -o $@ $< > > >> + cmd_cc_ll_c = $(CC) $(c_flags) -emit-llvm -S -fno-discard-value-names -o $@ $< > > >> > > >> $(obj)/%.ll: $(src)/%.c FORCE > > >> $(call if_changed_dep,cc_ll_c) > > >> > > >> base-commit: 93ed07a23fd08b8613f64cf0a15d7fbdaca010fd > > >> -- > > >> 2.38.0.rc2.412.g84df46c1b4-goog > > >> > > > > > > > -- > Thanks, > ~Nick Desaulniers
diff --git a/scripts/Makefile.build b/scripts/Makefile.build index 27be77c0d6d8..d0e4f476dfee 100644 --- a/scripts/Makefile.build +++ b/scripts/Makefile.build @@ -140,7 +140,7 @@ $(obj)/%.symtypes : $(src)/%.c FORCE # LLVM assembly # Generate .ll files from .c quiet_cmd_cc_ll_c = CC $(quiet_modtag) $@ - cmd_cc_ll_c = $(CC) $(c_flags) -emit-llvm -S -o $@ $< + cmd_cc_ll_c = $(CC) $(c_flags) -emit-llvm -S -fno-discard-value-names -o $@ $< $(obj)/%.ll: $(src)/%.c FORCE $(call if_changed_dep,cc_ll_c)
When debugging LLVM IR, it can be handy for clang to not discard value names used for local variables and parameters. Compare the generated IR. Implicit Default (-fdiscard-value-names): define i32 @core_sys_select(i32 %0, ptr %1, ptr %2, ptr %3, ptr %4) { %6 = alloca i64 %7 = alloca %struct.poll_wqueues %8 = alloca [64 x i32] Explicit -fno-discard-value-names: define i32 @core_sys_select(i32 %n, ptr %inp, ptr %outp, ptr %exp, ptr %end_time) { %expire.i = alloca i64 %table.i = alloca %struct.poll_wqueues %stack_fds = alloca [64 x i32] The rule for generating human readable LLVM IR (.ll) is only useful as a debugging feature: $ make LLVM=1 fs/select.ll Clang defaults to -fdiscard-value-names to save memory when generating LLVM IR. For debugging purposes, the improvement in readability at a cost of more verbose IR is a cost we're happy to pay. Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> --- scripts/Makefile.build | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) base-commit: 93ed07a23fd08b8613f64cf0a15d7fbdaca010fd