[IR] Don't verify module flags on every access #102153
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
8b4306c introduced validity checks for every module flag access, because the auto-upgrader uses named metadata before verifying the module. This causes overhead for all other accesses, and the check is, in fact, only need at that single place. Change the upgrader to be careful when accessing module flags before the module is verified and remove the checks on all other occasions. There are two tangential optimizations included: first, when querying a specific flag, don't enumerate all other flags into a vector as well. Second, don't use a Twine for getNamedMetadata(), which has materialization overhead -- all call sites use simple strings that can be implicitly converted to a StringRef.
|
@llvm/pr-subscribers-llvm-ir Author: Alexis Engelke (aengelke) Changes8b4306c introduced validity checks for every module flag access, because the auto-upgrader uses named metadata before verifying the module. This causes overhead for all other accesses, and the check is, in fact, only need at that single place. Change the upgrader to be careful when accessing module flags before the module is verified and remove the checks on all other occasions. There are two tangential optimizations included: first, when querying a specific flag, don't enumerate all other flags into a vector as well. Second, don't use a Twine for getNamedMetadata(), which has materialization overhead -- all call sites use simple strings that can be implicitly converted to a StringRef. c-t-t -0.19% in stage2-O3, -0.14% in stage2-O0-g Full diff: https://github.com/llvm/llvm-project/pull/102153.diff 3 Files Affected:
diff --git a/llvm/include/llvm/IR/Module.h b/llvm/include/llvm/IR/Module.h
index d2b2fe40b1ada..7042a54a59e7d 100644
--- a/llvm/include/llvm/IR/Module.h
+++ b/llvm/include/llvm/IR/Module.h
@@ -158,11 +158,6 @@ class LLVM_EXTERNAL_VISIBILITY Module {
/// converted result in MFB.
static bool isValidModFlagBehavior(Metadata *MD, ModFlagBehavior &MFB);
- /// Check if the given module flag metadata represents a valid module flag,
- /// and store the flag behavior, the key string and the value metadata.
- static bool isValidModuleFlag(const MDNode &ModFlag, ModFlagBehavior &MFB,
- MDString *&Key, Metadata *&Val);
-
struct ModuleFlagEntry {
ModFlagBehavior Behavior;
MDString *Key;
@@ -502,7 +497,7 @@ class LLVM_EXTERNAL_VISIBILITY Module {
/// Return the first NamedMDNode in the module with the specified name. This
/// method returns null if a NamedMDNode with the specified name is not found.
- NamedMDNode *getNamedMetadata(const Twine &Name) const;
+ NamedMDNode *getNamedMetadata(StringRef Name) const;
/// Return the named MDNode in the module with the specified name. This method
/// returns a new NamedMDNode if a NamedMDNode with the specified name is not
diff --git a/llvm/lib/IR/AutoUpgrade.cpp b/llvm/lib/IR/AutoUpgrade.cpp
index 53de9eef516b3..ec719754183d5 100644
--- a/llvm/lib/IR/AutoUpgrade.cpp
+++ b/llvm/lib/IR/AutoUpgrade.cpp
@@ -4886,7 +4886,25 @@ bool llvm::UpgradeDebugInfo(Module &M) {
if (DisableAutoUpgradeDebugInfo)
return false;
- unsigned Version = getDebugMetadataVersionFromModule(M);
+ // We need to get metadata before the module is verified (i.e., getModuleFlag
+ // makes assumptions that we haven't verified yet). Carefully extract the flag
+ // from the metadata.
+ unsigned Version = 0;
+ if (NamedMDNode *ModFlags = M.getModuleFlagsMetadata()) {
+ auto OpIt = find_if(ModFlags->operands(), [](const MDNode *Flag) {
+ if (Flag->getNumOperands() < 3)
+ return false;
+ if (MDString *K = dyn_cast_or_null<MDString>(Flag->getOperand(1)))
+ return K->getString() == "Debug Info Version";
+ return false;
+ });
+ if (OpIt != ModFlags->op_end()) {
+ const MDOperand &ValOp = (*OpIt)->getOperand(2);
+ if (auto *CI = mdconst::dyn_extract_or_null<ConstantInt>(ValOp))
+ Version = CI->getZExtValue();
+ }
+ }
+
if (Version == DEBUG_METADATA_VERSION) {
bool BrokenDebugInfo = false;
if (verifyModule(M, &llvm::errs(), &BrokenDebugInfo))
diff --git a/llvm/lib/IR/Module.cpp b/llvm/lib/IR/Module.cpp
index c966c53d09baf..80b5408b61eda 100644
--- a/llvm/lib/IR/Module.cpp
+++ b/llvm/lib/IR/Module.cpp
@@ -259,10 +259,8 @@ GlobalIFunc *Module::getNamedIFunc(StringRef Name) const {
/// getNamedMetadata - Return the first NamedMDNode in the module with the
/// specified name. This method returns null if a NamedMDNode with the
/// specified name is not found.
-NamedMDNode *Module::getNamedMetadata(const Twine &Name) const {
- SmallString<256> NameData;
- StringRef NameRef = Name.toStringRef(NameData);
- return NamedMDSymTab.lookup(NameRef);
+NamedMDNode *Module::getNamedMetadata(StringRef Name) const {
+ return NamedMDSymTab.lookup(Name);
}
/// getOrInsertNamedMetadata - Return the first named MDNode in the module
@@ -296,20 +294,6 @@ bool Module::isValidModFlagBehavior(Metadata *MD, ModFlagBehavior &MFB) {
return false;
}
-bool Module::isValidModuleFlag(const MDNode &ModFlag, ModFlagBehavior &MFB,
- MDString *&Key, Metadata *&Val) {
- if (ModFlag.getNumOperands() < 3)
- return false;
- if (!isValidModFlagBehavior(ModFlag.getOperand(0), MFB))
- return false;
- MDString *K = dyn_cast_or_null<MDString>(ModFlag.getOperand(1));
- if (!K)
- return false;
- Key = K;
- Val = ModFlag.getOperand(2);
- return true;
-}
-
/// getModuleFlagsMetadata - Returns the module flags in the provided vector.
void Module::
getModuleFlagsMetadata(SmallVectorImpl<ModuleFlagEntry> &Flags) const {
@@ -317,25 +301,24 @@ getModuleFlagsMetadata(SmallVectorImpl<ModuleFlagEntry> &Flags) const {
if (!ModFlags) return;
for (const MDNode *Flag : ModFlags->operands()) {
- ModFlagBehavior MFB;
- MDString *Key = nullptr;
- Metadata *Val = nullptr;
- if (isValidModuleFlag(*Flag, MFB, Key, Val)) {
- // Check the operands of the MDNode before accessing the operands.
- // The verifier will actually catch these failures.
- Flags.push_back(ModuleFlagEntry(MFB, Key, Val));
- }
+ // The verifier will catch errors, so no need to check them here.
+ auto *MFBConstant = mdconst::extract<ConstantInt>(Flag->getOperand(0));
+ auto MFB = static_cast<ModFlagBehavior>(MFBConstant->getLimitedValue());
+ MDString *Key = cast<MDString>(Flag->getOperand(1));
+ Metadata *Val = Flag->getOperand(2);
+ Flags.push_back(ModuleFlagEntry(MFB, Key, Val));
}
}
/// Return the corresponding value if Key appears in module flags, otherwise
/// return null.
Metadata *Module::getModuleFlag(StringRef Key) const {
- SmallVector<Module::ModuleFlagEntry, 8> ModuleFlags;
- getModuleFlagsMetadata(ModuleFlags);
- for (const ModuleFlagEntry &MFE : ModuleFlags) {
- if (Key == MFE.Key->getString())
- return MFE.Val;
+ const NamedMDNode *ModFlags = getModuleFlagsMetadata();
+ if (!ModFlags)
+ return nullptr;
+ for (const MDNode *Flag : ModFlags->operands()) {
+ if (Key == cast<MDString>(Flag->getOperand(1))->getString())
+ return Flag->getOperand(2);
}
return nullptr;
}
@@ -388,10 +371,7 @@ void Module::setModuleFlag(ModFlagBehavior Behavior, StringRef Key,
NamedMDNode *ModFlags = getOrInsertModuleFlagsMetadata();
// Replace the flag if it already exists.
for (MDNode *Flag : ModFlags->operands()) {
- ModFlagBehavior MFB;
- MDString *K = nullptr;
- Metadata *V = nullptr;
- if (isValidModuleFlag(*Flag, MFB, K, V) && K->getString() == Key) {
+ if (cast<MDString>(Flag->getOperand(1))->getString() == Key) {
Flag->replaceOperandWith(2, Val);
return;
}
|
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/30/builds/3447 Here is the relevant piece of the build log for the reference: |
searlmc1
pushed a commit
to ROCm/llvm-project
that referenced
this pull request
Aug 22, 2024
reverts: [IR] Don't verify module flags on every access (llvm#102153) Change-Id: I0c1ccc88f8617501d70cf402882145e8c5aa4d3c
searlmc1
pushed a commit
to ROCm/llvm-project
that referenced
this pull request
Sep 3, 2024
8b4306c introduced validity checks for every module flag access, because the auto-upgrader uses named metadata before verifying the module. This causes overhead for all other accesses, and the check is, in fact, only need at that single place. Change the upgrader to be careful when accessing module flags before the module is verified and remove the checks on all other occasions. There are two tangential optimizations included: first, when querying a specific flag, don't enumerate all other flags into a vector as well. Second, don't use a Twine for getNamedMetadata(), which has materialization overhead -- all call sites use simple strings that can be implicitly converted to a StringRef. (cherry picked from commit b7cd564) To minimize the diff I just copied the "checked" version of getDebugMetadataVersionFromModule that was inlined into UpgradeDebugInfo in the upstream patch. One could also delay the checks that rely on the version in the Verifier until after we've already verified the module flags metadata, but that seemed needlessly complicated. Change-Id: Id98ec158770d5b3e839e3de1c558f507c28cc82c
searlmc1
pushed a commit
to ROCm/llvm-project
that referenced
this pull request
Sep 7, 2024
8b4306c introduced validity checks for every module flag access, because the auto-upgrader uses named metadata before verifying the module. This causes overhead for all other accesses, and the check is, in fact, only need at that single place. Change the upgrader to be careful when accessing module flags before the module is verified and remove the checks on all other occasions. There are two tangential optimizations included: first, when querying a specific flag, don't enumerate all other flags into a vector as well. Second, don't use a Twine for getNamedMetadata(), which has materialization overhead -- all call sites use simple strings that can be implicitly converted to a StringRef. Change-Id: I724c67e52f080caba6bc16ec983f6118429408a1
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
8b4306c introduced validity checks for every module flag access, because the auto-upgrader uses named metadata before verifying the module.
This causes overhead for all other accesses, and the check is, in fact, only need at that single place. Change the upgrader to be careful when accessing module flags before the module is verified and remove the checks on all other occasions.
There are two tangential optimizations included: first, when querying a specific flag, don't enumerate all other flags into a vector as well. Second, don't use a Twine for getNamedMetadata(), which has materialization overhead -- all call sites use simple strings that can be implicitly converted to a StringRef.
c-t-t -0.19% in stage2-O3, -0.14% in stage2-O0-g