[LLVM] Add new attribute optdebug to optimize for debugging
#66632
Conversation
SLTozer
requested review from
echristo,
jmorse,
adrian-prantl,
OCHyams and
dwblaikie
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-debuginfo ChangesThis patch adds a new fn attribute, This patch does not add any functional changes triggered by this attribute, only the attribute itself. A subsequent patch will use this flag to disable the post-RA scheduler. I've added the reviewers of the original patch (D157615), i.e. debug info codeowners and the others who left comments; as far as I'm aware there isn't a codeowner for "attributes", but if there's some owned category that applies then lmk and I'll add the relevant reviewer.Full diff: https://github.com/llvm/llvm-project/pull/66632.diff 12 Files Affected:
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index 8b0c9340775cbe9..96d053a6aa8f9e5 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -2325,6 +2325,7 @@ void CodeGenModule::SetLLVMFunctionAttributesForDefinition(const Decl *D,
B.addAttribute(llvm::Attribute::Naked);
// OptimizeNone wins over OptimizeForSize and MinSize.
+ F->removeFnAttr(llvm::Attribute::OptimizeForDebugging);
F->removeFnAttr(llvm::Attribute::OptimizeForSize);
F->removeFnAttr(llvm::Attribute::MinSize);
} else if (D->hasAttr<NakedAttr>()) {
diff --git a/llvm/docs/BitCodeFormat.rst b/llvm/docs/BitCodeFormat.rst
index 70be73abef19d6d..ce0e29fb6b928a7 100644
--- a/llvm/docs/BitCodeFormat.rst
+++ b/llvm/docs/BitCodeFormat.rst
@@ -1085,6 +1085,7 @@ The integer codes are mapped to well-known attributes as follows.
* code 77: ``elementtype``
* code 78: ``disable_sanitizer_instrumentation``
* code 79: ``nosanitize_bounds``
+* code 88: ``optdebug``
.. note::
The ``allocsize`` attribute has a special encoding for its arguments. Its two
diff --git a/llvm/docs/LangRef.rst b/llvm/docs/LangRef.rst
index f542e70bcfee810..fa274fdb66a5047 100644
--- a/llvm/docs/LangRef.rst
+++ b/llvm/docs/LangRef.rst
@@ -2024,6 +2024,10 @@ example:
Note: Comparing address of a global variable to ``null`` may still
evaluate to false because of a limitation in querying this attribute inside
constant expressions.
+``optdebug``
+ This attribute suggests that optimization passes and code generator passes
+ should make choices that try to preserve debug info without significantly
+ degrading runtime performance.
``optforfuzzing``
This attribute indicates that this function should be optimized
for maximum fuzzing signal.
diff --git a/llvm/include/llvm/Bitcode/LLVMBitCodes.h b/llvm/include/llvm/Bitcode/LLVMBitCodes.h
index 52e76356a892e45..5d7be5ca936ad37 100644
--- a/llvm/include/llvm/Bitcode/LLVMBitCodes.h
+++ b/llvm/include/llvm/Bitcode/LLVMBitCodes.h
@@ -713,6 +713,7 @@ enum AttributeKindCodes {
ATTR_KIND_SKIP_PROFILE = 85,
ATTR_KIND_MEMORY = 86,
ATTR_KIND_NOFPCLASS = 87,
+ ATTR_KIND_OPTIMIZE_FOR_DEBUGGING = 88,
};
enum ComdatSelectionKindCodes {
diff --git a/llvm/include/llvm/IR/Attributes.td b/llvm/include/llvm/IR/Attributes.td
index aba1d718f7f72f9..fda79f5f24495fb 100644
--- a/llvm/include/llvm/IR/Attributes.td
+++ b/llvm/include/llvm/IR/Attributes.td
@@ -200,6 +200,9 @@ def NoSanitizeCoverage : EnumAttr<"nosanitize_coverage", [FnAttr]>;
/// Null pointer in address space zero is valid.
def NullPointerIsValid : EnumAttr<"null_pointer_is_valid", [FnAttr]>;
+/// Select optimizations that give decent debug info.
+def OptimizeForDebugging : EnumAttr<"optdebug", [FnAttr]>;
+
/// Select optimizations for best fuzzing signal.
def OptForFuzzing : EnumAttr<"optforfuzzing", [FnAttr]>;
diff --git a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
index 1d1ec988a93d847..16eafa6e18f5d59 100644
--- a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
+++ b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
@@ -1980,6 +1980,8 @@ static Attribute::AttrKind getAttrFromCode(uint64_t Code) {
return Attribute::NoSanitizeCoverage;
case bitc::ATTR_KIND_NULL_POINTER_IS_VALID:
return Attribute::NullPointerIsValid;
+ case bitc::ATTR_KIND_OPTIMIZE_FOR_DEBUGGING:
+ return Attribute::OptimizeForDebugging;
case bitc::ATTR_KIND_OPT_FOR_FUZZING:
return Attribute::OptForFuzzing;
case bitc::ATTR_KIND_OPTIMIZE_FOR_SIZE:
diff --git a/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp b/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
index f53fbd73667762c..116d035d4601b01 100644
--- a/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
+++ b/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
@@ -747,6 +747,8 @@ static uint64_t getAttrKindEncoding(Attribute::AttrKind Kind) {
return bitc::ATTR_KIND_NO_SANITIZE_COVERAGE;
case Attribute::NullPointerIsValid:
return bitc::ATTR_KIND_NULL_POINTER_IS_VALID;
+ case Attribute::OptimizeForDebugging:
+ return bitc::ATTR_KIND_OPTIMIZE_FOR_DEBUGGING;
case Attribute::OptForFuzzing:
return bitc::ATTR_KIND_OPT_FOR_FUZZING;
case Attribute::OptimizeForSize:
diff --git a/llvm/lib/Transforms/Utils/CodeExtractor.cpp b/llvm/lib/Transforms/Utils/CodeExtractor.cpp
index cafa99491f5b5f6..ef53b4ea0067243 100644
--- a/llvm/lib/Transforms/Utils/CodeExtractor.cpp
+++ b/llvm/lib/Transforms/Utils/CodeExtractor.cpp
@@ -941,6 +941,7 @@ Function *CodeExtractor::constructFunction(const ValueSet &inputs,
case Attribute::NoSanitizeBounds:
case Attribute::NoSanitizeCoverage:
case Attribute::NullPointerIsValid:
+ case Attribute::OptimizeForDebugging:
case Attribute::OptForFuzzing:
case Attribute::OptimizeNone:
case Attribute::OptimizeForSize:
diff --git a/llvm/test/Bitcode/attributes.ll b/llvm/test/Bitcode/attributes.ll
index 9af648fe262a351..eaf670575f4dd73 100644
--- a/llvm/test/Bitcode/attributes.ll
+++ b/llvm/test/Bitcode/attributes.ll
@@ -511,6 +511,12 @@ define void @f87() fn_ret_thunk_extern { ret void }
; CHECK: define void @f88() [[SKIPPROFILE:#[0-9]+]]
define void @f88() skipprofile { ret void }
+define void @f89() optdebug
+; CHECK: define void @f89() [[OPTDEBUG:#[0-9]+]]
+{
+ ret void;
+}
+
; CHECK: attributes #0 = { noreturn }
; CHECK: attributes #1 = { nounwind }
; CHECK: attributes #2 = { memory(none) }
@@ -566,4 +572,5 @@ define void @f88() skipprofile { ret void }
; CHECK: attributes #52 = { nosanitize_bounds }
; CHECK: attributes [[FNRETTHUNKEXTERN]] = { fn_ret_thunk_extern }
; CHECK: attributes [[SKIPPROFILE]] = { skipprofile }
+; CHECK: attributes [[OPTDEBUG]] = { optdebug }
; CHECK: attributes #[[NOBUILTIN]] = { nobuiltin }
diff --git a/llvm/utils/emacs/llvm-mode.el b/llvm/utils/emacs/llvm-mode.el
index e37cc693a1940aa..53381cf91b17b90 100644
--- a/llvm/utils/emacs/llvm-mode.el
+++ b/llvm/utils/emacs/llvm-mode.el
@@ -25,7 +25,7 @@
'("alwaysinline" "argmemonly" "allocsize" "builtin" "cold" "convergent" "dereferenceable" "dereferenceable_or_null" "hot" "immarg" "inaccessiblememonly"
"inaccessiblemem_or_argmemonly" "inalloca" "inlinehint" "jumptable" "minsize" "mustprogress" "naked" "nobuiltin" "nonnull" "nocapture"
"nocallback" "nocf_check" "noduplicate" "nofree" "noimplicitfloat" "noinline" "nomerge" "nonlazybind" "noprofile" "noredzone" "noreturn"
- "norecurse" "nosync" "noundef" "nounwind" "nosanitize_bounds" "nosanitize_coverage" "null_pointer_is_valid" "optforfuzzing" "optnone" "optsize" "preallocated" "readnone" "readonly" "returned" "returns_twice"
+ "norecurse" "nosync" "noundef" "nounwind" "nosanitize_bounds" "nosanitize_coverage" "null_pointer_is_valid" "optdebug" "optforfuzzing" "optnone" "optsize" "preallocated" "readnone" "readonly" "returned" "returns_twice"
"shadowcallstack" "signext" "speculatable" "speculative_load_hardening" "ssp" "sspreq" "sspstrong" "safestack" "sanitize_address" "sanitize_hwaddress" "sanitize_memtag"
"sanitize_thread" "sanitize_memory" "strictfp" "swifterror" "uwtable" "vscale_range" "willreturn" "writeonly" "zeroext") 'symbols) . font-lock-constant-face)
;; Variables
diff --git a/llvm/utils/kate/llvm.xml b/llvm/utils/kate/llvm.xml
index 9f7ec77bf3154d4..0e7aec3880e6b4a 100644
--- a/llvm/utils/kate/llvm.xml
+++ b/llvm/utils/kate/llvm.xml
@@ -111,6 +111,7 @@
<item> nosync </item>
<item> nounwind </item>
<item> null_pointer_is_valid </item>
+ <item> optdebug </item>
<item> optforfuzzing </item>
<item> optnone </item>
<item> optsize </item>
diff --git a/llvm/utils/vim/syntax/llvm.vim b/llvm/utils/vim/syntax/llvm.vim
index 9185a029a22e570..d86e3d1ddbc27ff 100644
--- a/llvm/utils/vim/syntax/llvm.vim
+++ b/llvm/utils/vim/syntax/llvm.vim
@@ -142,6 +142,7 @@ syn keyword llvmKeyword
\ nosanitize_bounds
\ nosanitize_coverage
\ null_pointer_is_valid
+ \ optdebug
\ optforfuzzing
\ optnone
\ optsize
|
clang/lib/CodeGen/CodeGenModule.cpp
Outdated
There was a problem hiding this comment.
Given that this patch doesn't have any other clang changes, this one can be deferred to a later patch where clang actually applies the attribute.
There was a problem hiding this comment.
+1, and otherwise the change makes sense to me. Could you post about this on Discourse, pointing to this pull request - just to make sure folks are aware we're adding a new IR attribute here? Be good to ensure there's consensus.
There was a problem hiding this comment.
|
Are there interactions between |
+1, this is basically "I want |
|
And same with |
|
Making this attribute mutually exclusive with |
|
I think typically the verifier changes are done in the same change since they match the LangRef change, but it doesn't matter too much as long as it gets sdone. The LangRef change should also mention the attributes this is incompatible with (like |
llvm/docs/BitCodeFormat.rst
Outdated
There was a problem hiding this comment.
if you're going to update this, can you first add the attributes in between?
There was a problem hiding this comment.
This could/should be committed separately/before this change - since it's fixing an existing documentation problem, not directly related to the patch adding optdebug (eg: if we need to revert the optdebug patch for any reason, we wouldn't want to lose this work)
|
Small ping - all comments have now been addressed now I think. |
llvm/docs/BitCodeFormat.rst
Outdated
There was a problem hiding this comment.
This could/should be committed separately/before this change - since it's fixing an existing documentation problem, not directly related to the patch adding optdebug (eg: if we need to revert the optdebug patch for any reason, we wouldn't want to lose this work)
llvm/docs/LangRef.rst
Outdated
There was a problem hiding this comment.
Looks like this could be committed separately.
Separate review opened up at: #68967 For simplicity's sake I'll keep this patch as-is, and rebase it when the other patch lands. |
This patch adds a new fn attribute, `optdebug`, that specifies that optimizations should make decisions that prioritize debug info quality, potentially at the cost of runtime performance. This patch does not add any functional changes triggered by this attribute, only the attribute itself.
SLTozer
force-pushed
the
add-optdebug-attr
branch
from
4b3a15f to
58f171e
Compare
| performance in order to minimize the size of the generated code. | ||
| This attribute is incompatible with the ``optnone`` attribute. | ||
| This attribute is incompatible with the ``optdebug`` and ``optnone`` | ||
| attributes. |
There was a problem hiding this comment.
It seems that you're missing the Verifier change to enforce this?
This patch adds a new fn attribute,
optdebug, that specifies that optimizations should make decisions that prioritize debug info quality, potentially at the cost of runtime performance.This patch does not add any functional changes triggered by this attribute, only the attribute itself. A subsequent patch will use this flag to disable the post-RA scheduler.
I've added the reviewers of the original patch (D157615), i.e. debug info codeowners and the others who left comments; as far as I'm aware there isn't a codeowner for "attributes", but if there's some owned category that applies then lmk and I'll add the relevant reviewer.