Skip to content

[NFC][KeyInstr] Add (LLVM_)EXPERIMENTAL_KEY_INSTRUCTIONS (cmake/)definition #131344

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

Merged
merged 1 commit into from
Mar 17, 2025

Conversation

OCHyams
Copy link
Contributor

@OCHyams OCHyams commented Mar 14, 2025

Key Instructions will start development behind a compile time flag to avoid
passing on the increased memory usage to all debug builds. We're working on
improving DILocation memory characteristics simultaneously; once that work lands
we can remove EXPERIMENTAL_KEY_INSTRUCTIONS.

This patch doesn't add any code, it's just so we can get the SIE buildbot
building with the new option right away.

…nition

Key Instructions will start development behind a compile time flag to avoid
passing on the increased memory usage to all debug builds. We're working on
improving DILocation memory characteristics simultaneously; once that work
lands we can remove `EXPERIMENTAL_KEY_INSTRUCTIONS`.

This patch doesn't add any code, it's just so we can get the SIE buildbot
building with the new option right away.
@OCHyams OCHyams requested review from jmorse, SLTozer and dyung March 14, 2025 15:15
@llvmbot llvmbot added the cmake Build system in general and CMake in particular label Mar 14, 2025
@llvmbot
Copy link
Member

llvmbot commented Mar 14, 2025

@llvm/pr-subscribers-debuginfo

Author: Orlando Cazalet-Hyams (OCHyams)

Changes

Key Instructions will start development behind a compile time flag to avoid
passing on the increased memory usage to all debug builds. We're working on
improving DILocation memory characteristics simultaneously; once that work lands
we can remove EXPERIMENTAL_KEY_INSTRUCTIONS.

This patch doesn't add any code, it's just so we can get the SIE buildbot
building with the new option right away.


Full diff: https://github.com/llvm/llvm-project/pull/131344.diff

2 Files Affected:

  • (modified) llvm/CMakeLists.txt (+3)
  • (modified) llvm/cmake/modules/HandleLLVMOptions.cmake (+4)
diff --git a/llvm/CMakeLists.txt b/llvm/CMakeLists.txt
index c5d3e23a47f0e..18b6ee85fae8d 100644
--- a/llvm/CMakeLists.txt
+++ b/llvm/CMakeLists.txt
@@ -545,6 +545,9 @@ set(LLVM_ENABLE_DEBUGLOC_COVERAGE_TRACKING "DISABLED" CACHE STRING
   "Enhance Debugify's line number coverage tracking; enabling this is ABI-breaking. Can be DISABLED, or COVERAGE.")
 set_property(CACHE LLVM_ENABLE_DEBUGLOC_COVERAGE_TRACKING PROPERTY STRINGS DISABLED COVERAGE)
 
+option(LLVM_EXPERIMENTAL_KEY_INSTRUCTIONS
+  "Add additional fields to DILocations to support Key Instructions" OFF)
+
 set(WINDOWS_PREFER_FORWARD_SLASH_DEFAULT OFF)
 if (MINGW)
   # Cygwin doesn't identify itself as Windows, and thus gets path::Style::posix
diff --git a/llvm/cmake/modules/HandleLLVMOptions.cmake b/llvm/cmake/modules/HandleLLVMOptions.cmake
index 5db06ccdadbeb..c2c04051ef3ce 100644
--- a/llvm/cmake/modules/HandleLLVMOptions.cmake
+++ b/llvm/cmake/modules/HandleLLVMOptions.cmake
@@ -206,6 +206,10 @@ else()
   message(FATAL_ERROR "Unknown value for LLVM_ENABLE_DEBUGLOC_COVERAGE_TRACKING: \"${LLVM_ENABLE_DEBUGLOC_COVERAGE_TRACKING}\"!")
 endif()
 
+if(LLVM_EXPERIMENTAL_KEY_INSTRUCTIONS)
+  add_compile_definitions(EXPERIMENTAL_KEY_INSTRUCTIONS)
+endif()
+
 if( LLVM_REVERSE_ITERATION )
   set( LLVM_ENABLE_REVERSE_ITERATION 1 )
 endif()

Copy link
Member

@jmorse jmorse left a comment

Choose a reason for hiding this comment

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

LGTM; this is how the RemoveDIs prototype was implemented, hopefully like RemoveDIs it'll avoid un-necessary interference with other folks while still getting test coverage.

@OCHyams
Copy link
Contributor Author

OCHyams commented Mar 17, 2025

I hope it's not too pushy in landing this, as it's not clear the RFC for key instructions is "accepted". This can easily be reverted; I'm just trying to set up the testing path.

@OCHyams OCHyams merged commit f4feab9 into llvm:main Mar 17, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmake Build system in general and CMake in particular debuginfo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants