LGTM; I always have to re-learn what the combinations of uwtable and nounwind mean, but this seems reasonable to me

LGTM, thank you again for attempting the switch to doing this unconditionally! I was clearly wrong about there being no dependence if even an in-tree project is breaking

(Cross-posting from the other thread, to keep everything in one place)

I took a stab at a patch to address the assertion at D150713

In D147270#4337447, @CarlosAlbertoEnciso wrote:

In D147270#4327735, @scott.linder wrote:

Is there a good target you have in mind for the benchmark? I seem to hit an assertion when dogfooding RelWithDebInfo binaries:

$ build/bin/llvm-debuginfo-analyzer --print=symbols build-relwithdebinfo/bin/llvm-tblgen
llvm-debuginfo-analyzer: /home/slinder1/llvm-project/main/llvm/include/llvm/DebugInfo/LogicalView/Core/LVSymbol.h:102: virtual v
oid llvm::logicalview::LVSymbol::setReference(llvm::logicalview::LVElement *): Assertion `(!Element || isa<LVSymbol>(Element)) &
& "Invalid element"' failed.

I've also tried `llc`, `clang`, and `llvm-debuginfo-analyzer` itself, all hit the same assert. Is there something I might be doing wrong on my end?

Sorry for my delay in answering but I was at the LLVM Euro 2023. Your command line is correct.

At Sony, we have managed to reproduce the same issue with a debug/checking build on a private project.

Does GCC have the same -ftime-trace= option? It seems like it doesn't, as https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92396 is still open?

In D149193#4316293, @dblaikie wrote:

I guess my main question is: What's the motivation for implementing this? Do you have a need/use for this? (it doesn't seem to be motivated by GCC compatibility - as discussed, looks like we're diverging in a bunch of ways from their behavior and the argument made that these are "developer" flags, so not a stable/compatible interface used across both compilers)

In D147270#4304833, @CarlosAlbertoEnciso wrote:

@scott.linder: Sorry for my delay. As pointed out by @dblaikie, it would be interesting to see the memory usage, specially for the logical view. Thanks.

I am OK to give LGTM, assuming the other reviewers don't still have reservations?

In D149193#4300885, @MaskRay wrote:

I think the patch as-is implements all the useful parts of GCC's complex rules and in the absence of -dumpbase (we deliberately don't implement), the rule almost exactly matches GCC unless we do gcc -g -gsplit-dwarf d/a.c -o e/a.out (instead of other filenames).

https://maskray.me/blog/2023-04-25-compiler-output-files records all my findings.

I'm a bit confused after trying to work out the rules for the GCC version of -dumpdir over at https://gcc.gnu.org/onlinedocs/gcc/Overall-Options.html#index-dumpdir but it at least seems like our version is a subset of theirs.

LGTM, thank you!

Now that we are no longer contrained by constexpr, just use
SmallVector for everything.

Determined constexpr was not useful here

In D147265#4241322, @dblaikie wrote:

Oh, naming though - DW_OP_LLVM_extension, perhaps? But I don't mind too much - the name isn't load bearing anyway, we can always rename it if we find a more descriptive name idf we're preserving the semantics

LGTM with 1 small nit

LGTM, thank you!

In D143463#4266127, @khuey wrote:

I'm happy to update this to do 1 + 2 + 3 too, it's not hard.

In D143463#4263367, @khuey wrote:

In D143463#4262748, @scott.linder wrote:

Very reasonable, but if GCC is already using the CFA unconditionally it seems likely that implies a pretty good support base?

Indeed.

I meant to propose another middle-ground, where we use the CFA whenever !hasFP, which (IIUC) is a precondition for x86 ever adjusting the CFA. It would mean we use the CFA more than with the heuristic, but still not unconditionally.

!hasFP is a precondition for x86 emitting CFA offset adjustment directives solely because if there is a frame pointer LLVM simply defines the CFA to be the frame pointer. And then the frame pointer remains the same regardless of adjustments to %rsp, so the CFA doesn't need to be adjusted simultaneously. See the createDefCfaRegister call inside X86FrameLowering::emitPrologue (which is gated on, at the top level, HasFP).

I think I am mostly concerned with the statefulness of the heuristic, coupled with the fact that it seems to not be completely contained. The majority of it is handled by X86FrameLowering, but there is at least the one instance where it has to be explicitly managed in X86MCInstLower. If others are not as concerned with this I'm happy to concede on it, but it is the main concern I have.

The not-being-contained is just because X86::MOVPC32r is really weird.

OK, I may have just been overly concerned about this; my familiarity with the X86 target in LLVM is very limited.

I definitely understand the complexity in DwarfExpression::addMachineRegExpression makes any change very difficult. I am happy with continuing to apply the offset to the CFA in the frame base, and if some other brave soul wants to change that in a future patch they won't be any worse off.

Yeah I don't really want to do that here but I agree that folding the offset between the CFA and the stack pointer into the variable expressions rather than leaving it in the DW_AT_frame_base expression is the ideal end point.

My current leaning is to change to using the CFA unconditionally, but offset to maintain the same frame base value. It will result in a slightly larger expression in the default case, but I would be much more confident that the result is always correct.

So, to be clear, you're learning towards using a CFA-based DW_AT_frame_base in all three of these scenarios?

1. There is a CFA, and LLVM is currently using %rsp as the DW_AT_frame_base, and %rsp changes throughout the function.
2. There is a CFA, and LLVM is currently using %rsp as the DW_AT_frame_base, and %rsp remains constant (after the prologue).
3. There is a CFA, and LLVM is currently using %rbp as the DW_AT_frame_base.

Where using the CFA for 1 is what this patch as originally proposed does, and 1 + 2 is the "middle ground" you suggested previously, as I understand it.

I think using the CFA for 1 + 2 + 3 makes more sense than doing just 1 + 2, so I think we more or less agree here.

In D143463#4259947, @khuey wrote:

In D143463#4259475, @scott.linder wrote:

As a bit of a side-note, I think X86 always maintains a "precise CFA" to refer to, but that may not always be true (see https://reviews.llvm.org/D14948).

This is relying on the CFA being correct when it's present. CFI is not always present (see X86FrameLowering::needsDwarfCFI which at a minimum excludes some Win64 stuff) but it might be present in all cases that matter. I'm not sure.

I'm curious about a couple points:

• If we do always have the CFA available, then is there any reason not to use it unconditionally?

In decreasing order of persuasiveness to me

1. If the frame pointer is present (or the stack pointer remains unchanged) using it is simpler for the debug info consumer (as there's no need to consult the CFI and run through its state machine). In theory there might also be old tools out there that don't support CFI as well.

Very reasonable, but if GCC is already using the CFA unconditionally it seems likely that implies a pretty good support base?

2. If the frame pointer is present (or the stack pointer remains unchanged) using it works today. Don't fix what isn't broke.

I definitely think this holds a lot of weight, but just having one concept (CFA) instead of several (stack-pointer, frame-pointer, frame-base, CFA) is also very attractive. The fact that the patch is even necessary also demonstrates something is broken, although it might be the exception that proves the rule.

3. If there are any bugs out there that result in an incorrect CFA the machine registers are more likely to be correct as they're actually used by the program and thus issues are more likely to be noticed.

If the CFA is incorrect, unwinding seems like it is doomed to fail, which should constitute a program execution bug if e.g. exceptions are enabled, right? I don't necessarily think we will end up with more elusive debug-info bugs as a result of relying on the CFA.

Although it seems the gcc maintainers were not persuaded by any of this. gcc emits DW_OP_call_frame_cfa in every circumstance I can think of (at least on x86-64).

• If we cannot use it unconditionally, can the condition be more direct, i.e. can we remove the heuristic?

I'm not aware of any reason we cannot use the CFA unconditionally (at least on x86) if it is present. I believe it is possible to remove the setHasCFIAdjustCfa heuristic and use the CFA in place of the stack pointer unconditionally if the above arguments are not persuasive. I believe it's also possible to use the CFA in place of the frame pointer even when it's present, if desired.

I meant to propose another middle-ground, where we use the CFA whenever !hasFP, which (IIUC) is a precondition for x86 ever adjusting the CFA. It would mean we use the CFA more than with the heuristic, but still not unconditionally.

Thank you for the patch! Sorry for the long delay in review

In D147267#4246564, @scott.linder wrote:

In D147267#4241300, @dblaikie wrote:

Sure - though could you check if there's any revision history that explains the comment further/motivates why this would be valuable? (I have some apprehension about making extra things constexpr as it can increase compile time excessively (forcing the frontend to check for evaluation - when the backend can more cheaply optimize it away later anyway))

Sure, I did just blindly assume the TODO had merit. Is there a structured way you tend to evaluate these on a case-by-case basis? Should I compare instructions retired while compiling LLVM+Clang and while using the resulting LLVM+Clang to inspect DebugInfo with and without the change?

Edit:

I compared building the lib/DebugInfo/DWARF/all target without the patch vs. with the patch, with some minimal isolation and attempts to make the result statistically significant without spending too much time:

• I ran everything out of a linux tmpfs
• I used ninja as the build system
• I'm compiling with clang version 11.0.0 (https://github.com/llvm/llvm-project 176249bd6732a8044d457092ed932768724a6f06)
• I ran the build six times, cleaning between each and throwing out the first run

<stat>: <control> <constexpr> (<constexpr>/<control>)
lib/libLLVMDebugInfoDWARF.a-size: 2021132 2016068 (0.997494)
instructions:u: 4271564407084 4271164683246 (0.999906)
Maximum resident set size: 694690 694689 (0.999999)
branches: 495120306752 495185537556 (1.000132)
branch-misses: 30440381987 30515429111 (1.002465)
User time: 2321.876000 2331.440000 (1.004119)

If https://www.npopov.com/2020/05/10/Make-LLVM-fast-again.html is to be believed the user instructions retired and max-rss metrics are the best proxies available, but I included some of the "noisy" metrics too. None of them seem too outrageously out-of-line, but I can rerun to see if e.g. the 0.4% increase user-time is a fluke.

I stole the cmake, time, and perf command-lines from the repo for http://llvm-compile-time-tracker.com/ referenced from the same blog post above. I haven't done much in this area so I don't know if I made any mistakes; my script is:

#!/bin/bash

build() {
  for f in control constexpr; do
    rm -rf build-$f
    mkdir -p build-$f
    rm -rf out-$f
    mkdir -p out-$f
    cmake \
      -GNinja \
      -S./source-$f/llvm \
      -B./build-$f \
      -DLLVM_ENABLE_PROJECTS="clang" \
      -DLLVM_TARGETS_TO_BUILD="X86" \
      -DLLVM_BUILD_TOOLS=false \
      -DLLVM_APPEND_VC_REV=false \
      -DCMAKE_BUILD_TYPE=Release \
      -DLLVM_CCACHE_BUILD=false \
      -DLLVM_USE_LINKER=gold \
      -DLLVM_BINUTILS_INCDIR=/usr/include \
      -DCLANG_ENABLE_ARCMT=false \
      -DCLANG_ENABLE_STATIC_ANALYZER=false
    /usr/bin/ninja -C build-$f lib/DebugInfo/DWARF/all
    for i in $(seq 5); do
      LC_ALL=C ninja -C build-$f clean
      LC_ALL=C \
        /usr/bin/time -v -o out-$f/time.$i \
        perf stat -x \; -o out-$f/perf.$i \
          -e instructions \
          -e instructions:u \
          -e cycles \
          -e task-clock \
          -e branches \
          -e branch-misses \
          ninja -C build-$f lib/DebugInfo/DWARF/all
    done
  done
}
print_stat_header() {
  printf "<stat>: <control> <constexpr> (<constexpr>/<control>)\n"
}
print_stat() {
  printf "%s: %s %s (%s)\n" "$1" "$2" "$3" "$(awk -- "BEGIN { printf \"%f\\n\", $3 / $2 }")"
}
size() {
  stat --printf='%s\n' "$@"
}
compare_size() {
  local file="$1"
  local control="$(size build-control/$file)"
  local constexpr="$(size build-constexpr/$file)"
  print_stat "$file-size" "$control" "$constexpr"
}
compare() {
  local dataset="$1"; shift
  local rowname="$1"; shift
  local fmt="$1"; shift
  local sep=:
  local idx=2
  if [[ $dataset == perf ]]; then
    sep=\;
    idx=1
  fi
  local awkprogram="/$rowname/ { sum += \$$idx; n++; } END { printf \"$fmt\\n\", sum / n; }"
  local control=$(awk -F "$sep" -- "$awkprogram" out-control/$dataset.*)
  local constexpr=$(awk -F "$sep" -- "$awkprogram" out-constexpr/$dataset.*)
  local relative=$(awk -- "BEGIN { printf \"%f\\n\", $constexpr / $control }")
  print_stat "$rowname" "$control" "$constexpr"
}
print() {
  print_stat_header
  compare_size lib/libLLVMDebugInfoDWARF.a
  compare perf 'instructions:u' '%u'
  compare time 'Maximum resident set size' '%u'
  compare perf 'branches' '%u'
  compare perf 'branch-misses' '%u'
  compare time 'User time' '%f'
}
main() {
  build
  print
}
"$@"

Run as bench.sh main

I will expand this to building the whole of llvm, and then dogfood the result, although I may not run as many trials to avoid it taking too long.

In D147270#4241303, @dblaikie wrote:

Got any measurements of memory usage? (seems like this'd increase memory usage? Maybe try a self-host release build (optimizations causing more interesting locations and so more location operations) & see how peak memory usage in various compilation changes?)

In D147269#4241315, @dblaikie wrote:

Not the best error handling (like might be better returning Error from this function?) but probably OK.

In D147267#4241300, @dblaikie wrote:

Sure - though could you check if there's any revision history that explains the comment further/motivates why this would be valuable? (I have some apprehension about making extra things constexpr as it can increase compile time excessively (forcing the frontend to check for evaluation - when the backend can more cheaply optimize it away later anyway))

I've also submitted a proposal for a standard version of the operation (thread at https://lists.dwarfstd.org/pipermail/dwarf-discuss/2023-March/002199.html)

Abandoned in favor of D147279 which avoids using most/all of the remaining user opcode

Abandoned in favor of D147279 which avoids using most/all of the remaining user opcode

Abandoned in favor of D147279 which avoids using most/all of the remaining user opcode

Rebase

In D145558#4199730, @Orlando wrote:

Thank you @scott.linder for taking a look at this.

It might be worth including the rationale in the description/commit message, like you've done in other similar changes. I assume it is a performance improvement?

Yep there's an average of around 0.25% reduction in instructions retired and a 1.1% max-rss for the CTMark suite in LTO-O3-g builds (I'll update the description).

I'm leaving a bunch of edit suggestions, but feel free to ignore them! I was just going through the exercise of writing one possible implementation to prove to myself it is an actual improvement, YMMV

I appreciate the in-depth review and the suggestions all look good to me!

The diff has become quite large now so I'll could look at trying to factor out some of the suggestions into NFC patches?

It might be worth including the rationale in the description/commit message, like you've done in other similar changes. I assume it is a performance improvement?

Is there a corresponding document update to go along with this? If consumers of the bundles will begin to rely on this it should be documented that it is guaranteed by the producer.

Address feedback:

• Fix test name to reflect it testing both r600 and amdcn
• Common up some checks between r600 and amdgcn
• Adjust some whitespace to make test more readable

Address feedback

In D144301#4160749, @jhenderson wrote:

It looks like this code is testing code in libObject, but the testing is in llvm-dwarfdump. Should the tests be moved to test/Object?

Ping

Seems reasonable to me! LGTM with a small nit

Address feedback

In D144301#4138183, @jhenderson wrote:

The patch title and code are to do with llvm-dwarfdump, but the commit message refers to llvm-objdump. Is this just a mistake?

This is great, I also didn't realize there were such a thing :)

Sorry for the delay in reviewing! LGTM with a couple nits

Are there any thoughts on whether this is too ugly to live? It will be awkward to teach users the current default behavior without this change, but if we can accept it as a historical quirk that may be OK.

Rebase

• Drop [Clang] from commit message.
• ltocgo -> ltoCgo

Replace asserts added in previous version of patch with true diagnostic errors.
The only excpetion is COFF, where the parsing still deals in integral values
and ensures they fall in the right range, which the Driver assumes/asserts.