Page MenuHomePhabricator

[llvm-profgen] Do not cache the frame location stack during computing inlined context size
AcceptedPublic

Authored by wlei on Jun 29 2022, 2:56 PM.

Details

Reviewers
hoy
wenlei
Summary

In computeInlinedContextSizeForRange, the offset of range is only used one time, there is no need to cache the frame location stack.
Measured on one internal service binary, this can save 2GB memory usage and reduce a small run time (avoid one hash search).

Diff Detail

Unit TestsFailed

TimeTest
60,510 msx64 debian > Clang.Driver::arm-cortex-cpus-2.c
Script: -- : 'RUN: at line 8'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -target armv8a-linux-eabi -mcpu=cortex-a53+fp16 -### -c /var/lib/buildkite-agent/builds/llvm-project/clang/test/Driver/arm-cortex-cpus-2.c 2>&1 | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck --check-prefix CHECK-CORTEX-A53-FP16 /var/lib/buildkite-agent/builds/llvm-project/clang/test/Driver/arm-cortex-cpus-2.c
60,210 msx64 debian > Clang.Driver::emit-reproducer.c
Script: -- : 'RUN: at line 1'; rm -rf /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/test/Driver/Output/emit-reproducer.c.tmp && mkdir /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/test/Driver/Output/emit-reproducer.c.tmp
60,600 msx64 debian > Clang.Driver::fsanitize.c
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -target x86_64-linux-gnu -fsanitize=undefined -fsanitize-trap=undefined /var/lib/buildkite-agent/builds/llvm-project/clang/test/Driver/fsanitize.c -### 2>&1 | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/clang/test/Driver/fsanitize.c --check-prefix=CHECK-UNDEFINED-TRAP
60,670 msx64 debian > Clang.OpenMP::target_defaultmap_codegen_01.cpp
Script: -- : 'RUN: at line 8'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -cc1 -internal-isystem /var/lib/buildkite-agent/builds/llvm-project/build/lib/clang/15.0.0/include -nostdsysteminc -no-opaque-pointers -no-enable-noundef-analysis -DCK1 -verify -fopenmp -fopenmp-version=50 -fopenmp-targets=powerpc64le-ibm-linux-gnu -x c++ -triple powerpc64le-unknown-unknown -emit-llvm /var/lib/buildkite-agent/builds/llvm-project/clang/test/OpenMP/target_defaultmap_codegen_01.cpp -o - | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck --allow-unused-prefixes -allow-deprecated-dag-overlap /var/lib/buildkite-agent/builds/llvm-project/clang/test/OpenMP/target_defaultmap_codegen_01.cpp --check-prefix CK1
60,630 msx64 debian > Clang.OpenMP::target_update_codegen.cpp
Script: -- : 'RUN: at line 6'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -cc1 -internal-isystem /var/lib/buildkite-agent/builds/llvm-project/build/lib/clang/15.0.0/include -nostdsysteminc -no-opaque-pointers -DCK1 -verify -fopenmp -fopenmp-targets=powerpc64le-ibm-linux-gnu -x c++ -triple powerpc64le-unknown-unknown -emit-llvm /var/lib/buildkite-agent/builds/llvm-project/clang/test/OpenMP/target_update_codegen.cpp -o - | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck --allow-unused-prefixes /var/lib/buildkite-agent/builds/llvm-project/clang/test/OpenMP/target_update_codegen.cpp --check-prefix CK1 --check-prefix CK1-64

Event Timeline

wlei created this revision.Jun 29 2022, 2:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2022, 2:56 PM
Herald added subscribers: hoy, wenlei. · View Herald Transcript
wlei requested review of this revision.Jun 29 2022, 2:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2022, 2:56 PM
wlei edited the summary of this revision. (Show Details)Jun 29 2022, 3:05 PM
wlei added reviewers: hoy, wenlei.
hoy added a comment.Jul 7 2022, 5:00 PM

Nice catch!

llvm/tools/llvm-profgen/ProfiledBinary.h
496–503

How about extend this function with a parameter for caching or not?

wlei added inline comments.Jul 11 2022, 9:30 AM
llvm/tools/llvm-profgen/ProfiledBinary.h
496–503

It seems the return types of the two functions should be different, the one without caching should return the value type instead of the reference as we don't store it to a map.

hoy added inline comments.Jul 11 2022, 11:15 AM
llvm/tools/llvm-profgen/ProfiledBinary.h
496–503

I see. Then it makes sense to leave them separate.

How about flip the names, i.e

getFrameLocationStackWithoutCaching -> getFrameLocationStack

getFrameLocationStack -> getCachedFrameLocation

Caching is expansive so we want users to be explicitly aware of it. Also perhaps in the future we want to use two containers for cached contexts, one for UseProbeDiscriminator==true and one for UseProbeDiscriminator==false.

wlei added inline comments.Jul 11 2022, 4:01 PM
llvm/tools/llvm-profgen/ProfiledBinary.h
496–503

Renamed, thanks for the suggestion!

wlei updated this revision to Diff 443782.Jul 11 2022, 4:01 PM

renaming according to reviewer's suggestion

hoy accepted this revision.Jul 11 2022, 4:36 PM
This revision is now accepted and ready to land.Jul 11 2022, 4:36 PM
hoy added inline comments.Jul 11 2022, 4:37 PM
llvm/tools/llvm-profgen/ProfiledBinary.h
502

nit: getCachedFrameLocation -> getCachedFrameLocationStack

Sorry for missing it previously.

wlei updated this revision to Diff 443793.Jul 11 2022, 5:30 PM

Updating D128859: [llvm-profgen] Do not cache the frame location stack during computing inlined context size