Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a pass to collect dropped var stats for MIR #120780

Merged
merged 1 commit into from
Dec 20, 2024

Conversation

rastogishubham
Copy link
Contributor

This patch uses the DroppedVariableStats class to add dropped variable statistics for MIR passes.

Reland 1c082c9

This patch uses the DroppedVariableStats class to add dropped variable
statistics for MIR passes.

Reland 1c082c9
@rastogishubham rastogishubham merged commit 3bf91ad into llvm:main Dec 20, 2024
6 of 8 checks passed
@rastogishubham rastogishubham deleted the DroppedVarStatsMIR branch December 20, 2024 18:08
rastogishubham added a commit to rastogishubham/llvm-project that referenced this pull request Dec 20, 2024
This patch uses the DroppedVariableStats class to add dropped variable
statistics for MIR passes.

Reland 1c082c9

(cherry picked from commit 3bf91ad)
Copy link
Contributor

@chapuni chapuni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should've moved DroppedVariableStats stuff out of llvm/Passes in advance.
At a glance, they don't depend on LLVMPass any more. (#120711)

#define LLVM_CODEGEN_DROPPEDVARIABLESTATSMIR_H

#include "llvm/CodeGen/MachineFunction.h"
#include "llvm/Passes/DroppedVariableStats.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's odd to depend on LLVMPasses here.

chapuni added a commit that referenced this pull request Dec 21, 2024
This reverts commit 3bf91ad.
(llvmorg-20-init-16123-g3bf91ad2a9c7)

`llvm/CodeGen` should not depend on `llvm/Passes`.
@chapuni
Copy link
Contributor

chapuni commented Dec 21, 2024

I have reverted this. I could fix them though.

@nikic
Copy link
Contributor

nikic commented Dec 21, 2024

I have already commented on this three times...

#115566 (comment)
#117044 (comment)
#120501 (comment)

...and my feedback still hasn't been addressed.

If you reapply this again without actually addressing the problems with this patch, I will request your commit access to be withdrawn.

@rastogishubham
Copy link
Contributor Author

@nikic I am very sorry about not addressing the patch issues, I will make sure to not include so much code into MachineFunctionPass.h. I had applied some aggressive filtering in my email and didn't see those comments, but I understand it is not an excuse.

Do you have some recommendations on how to redesign this better to make it so that the build times are smaller?

@rastogishubham
Copy link
Contributor Author

@chapuni The reason I designed it this way, is because if I move all the class DroppedVariableStats code out of llvm/Passes to codegen, then, the libclang/codegen library fails when linking on clang-ppc64le-linux-multistage

With this error:

/CodeGenModule.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/CodeGenPGO.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/CodeGenTBAA.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/CodeGenTypes.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/ConstantInitBuilder.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/CoverageMappingGen.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/ItaniumCXXABI.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/LinkInModulesPass.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/MacroPPCallbacks.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/MicrosoftCXXABI.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/ModuleBuilder.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/ObjectFilePCHContainerWriter.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/PatternInit.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/SanitizerMetadata.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/SwiftCallingConv.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/TargetInfo.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/Targets/AArch64.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/Targets/AMDGPU.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/Targets/ARC.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/Targets/ARM.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/Targets/AVR.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/Targets/BPF.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/Targets/CSKY.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/Targets/DirectX.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/Targets/Hexagon.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/Targets/Lanai.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/Targets/LoongArch.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/Targets/M68k.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/Targets/MSP430.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/Targets/Mips.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/Targets/NVPTX.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/Targets/PNaCl.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/Targets/PPC.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/Targets/RISCV.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/Targets/SPIR.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/Targets/Sparc.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/Targets/SystemZ.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/Targets/TCE.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/Targets/VE.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/Targets/WebAssembly.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/Targets/X86.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/Targets/XCore.cpp.o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/VarBypassDetector.cpp.o  -Wl,-rpath,"\$ORIGIN/../lib:/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-multistage-test/clang-ppc64le-multistage/stage1/lib:"  lib/libclangFrontend.so.20.0git  lib/libclangSerialization.so.20.0git  lib/libLLVMCoverage.so.20.0git  lib/libLLVMFrontendDriver.so.20.0git  lib/libLLVMLTO.so.20.0git  lib/libLLVMPasses.so.20.0git  lib/libclangAnalysis.so.20.0git  lib/libLLVMFrontendHLSL.so.20.0git  lib/libclangAST.so.20.0git  lib/libclangLex.so.20.0git  lib/libclangBasic.so.20.0git  lib/libLLVMExtensions.so.20.0git  lib/libLLVMCoroutines.so.20.0git  lib/libLLVMHipStdPar.so.20.0git  lib/libLLVMipo.so.20.0git  lib/libLLVMFrontendOpenMP.so.20.0git  lib/libLLVMFrontendOffloading.so.20.0git  lib/libLLVMLinker.so.20.0git  lib/libLLVMIRPrinter.so.20.0git  lib/libLLVMInstrumentation.so.20.0git  lib/libLLVMCodeGenTypes.so.20.0git  lib
libLLVMObjCARCOpts.so.20.0git  lib/libLLVMScalarOpts.so.20.0git  lib/libLLVMAggressiveInstCombine.so.20.0git  lib/libLLVMInstCombine.so.20.0git  lib/libLLVMTarget.so.20.0git  lib/libLLVMTransformUtils.so.20.0git  lib/libLLVMBitWriter.so.20.0git  lib/libLLVMAnalysis.so.20.0git  lib/libLLVMProfileData.so.20.0git  lib/libLLVMObject.so.20.0git  lib/libLLVMIRReader.so.20.0git  lib/libLLVMBitReader.so.20.0git  lib/libLLVMCore.so.20.0git  lib/libLLVMMC.so.20.0git  lib/libLLVMTargetParser.so.20.0git  lib/libLLVMSupport.so.20.0git  lib/libLLVMDemangle.so.20.0git  -Wl,-rpath-link,/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-multistage-test/clang-ppc64le-multistage/stage1/lib && :
tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/BackendUtil.cpp.o:(.toc+0x258): undefined reference to `vtable for llvm::DroppedVariableStatsIR'

This is because libclang/codegen doesn't link against libllvm/codegen however, it does link against libllvm/passes. If I cannot have libllvm/codegen not link against passes, then I am unsure of where I can put the code so that I can calculate dropped statistics for both IR and MIR.

Do you have an idea of what could be done?

@llvm-ci
Copy link
Collaborator

llvm-ci commented Dec 23, 2024

LLVM Buildbot has detected a new failure on builder lld-x86_64-win running on as-worker-93 while building llvm at step 7 "test-build-unified-tree-check-all".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/146/builds/1904

Here is the relevant piece of the build log for the reference
Step 7 (test-build-unified-tree-check-all) failure: test (failure)
******************** TEST 'LLVM-Unit :: Support/./SupportTests.exe/38/87' FAILED ********************
Script(shard):
--
GTEST_OUTPUT=json:C:\a\lld-x86_64-win\build\unittests\Support\.\SupportTests.exe-LLVM-Unit-17800-38-87.json GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=87 GTEST_SHARD_INDEX=38 C:\a\lld-x86_64-win\build\unittests\Support\.\SupportTests.exe
--

Script:
--
C:\a\lld-x86_64-win\build\unittests\Support\.\SupportTests.exe --gtest_filter=ProgramEnvTest.CreateProcessLongPath
--
C:\a\lld-x86_64-win\llvm-project\llvm\unittests\Support\ProgramTest.cpp(160): error: Expected equality of these values:
  0
  RC
    Which is: -2

C:\a\lld-x86_64-win\llvm-project\llvm\unittests\Support\ProgramTest.cpp(163): error: fs::remove(Twine(LongPath)): did not return errc::success.
error number: 13
error message: permission denied



C:\a\lld-x86_64-win\llvm-project\llvm\unittests\Support\ProgramTest.cpp:160
Expected equality of these values:
  0
  RC
    Which is: -2

C:\a\lld-x86_64-win\llvm-project\llvm\unittests\Support\ProgramTest.cpp:163
fs::remove(Twine(LongPath)): did not return errc::success.
error number: 13
error message: permission denied




********************


rastogishubham added a commit to rastogishubham/llvm-project that referenced this pull request Jan 25, 2025
This patch uses the DroppedVariableStats class to add dropped variable
statistics for MIR passes.

Reland 1c082c9

(cherry picked from commit 3bf91ad)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants