Skip to content

Commit b7cd564

Browse files
authored
[IR] Don't verify module flags on every access (#102153)
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.
1 parent 1d2b6d9 commit b7cd564

File tree

3 files changed

+35
-42
lines changed

3 files changed

+35
-42
lines changed

llvm/include/llvm/IR/Module.h

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -158,11 +158,6 @@ class LLVM_EXTERNAL_VISIBILITY Module {
158158
/// converted result in MFB.
159159
static bool isValidModFlagBehavior(Metadata *MD, ModFlagBehavior &MFB);
160160

161-
/// Check if the given module flag metadata represents a valid module flag,
162-
/// and store the flag behavior, the key string and the value metadata.
163-
static bool isValidModuleFlag(const MDNode &ModFlag, ModFlagBehavior &MFB,
164-
MDString *&Key, Metadata *&Val);
165-
166161
struct ModuleFlagEntry {
167162
ModFlagBehavior Behavior;
168163
MDString *Key;
@@ -502,7 +497,7 @@ class LLVM_EXTERNAL_VISIBILITY Module {
502497

503498
/// Return the first NamedMDNode in the module with the specified name. This
504499
/// method returns null if a NamedMDNode with the specified name is not found.
505-
NamedMDNode *getNamedMetadata(const Twine &Name) const;
500+
NamedMDNode *getNamedMetadata(StringRef Name) const;
506501

507502
/// Return the named MDNode in the module with the specified name. This method
508503
/// returns a new NamedMDNode if a NamedMDNode with the specified name is not

llvm/lib/IR/AutoUpgrade.cpp

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4886,7 +4886,25 @@ bool llvm::UpgradeDebugInfo(Module &M) {
48864886
if (DisableAutoUpgradeDebugInfo)
48874887
return false;
48884888

4889-
unsigned Version = getDebugMetadataVersionFromModule(M);
4889+
// We need to get metadata before the module is verified (i.e., getModuleFlag
4890+
// makes assumptions that we haven't verified yet). Carefully extract the flag
4891+
// from the metadata.
4892+
unsigned Version = 0;
4893+
if (NamedMDNode *ModFlags = M.getModuleFlagsMetadata()) {
4894+
auto OpIt = find_if(ModFlags->operands(), [](const MDNode *Flag) {
4895+
if (Flag->getNumOperands() < 3)
4896+
return false;
4897+
if (MDString *K = dyn_cast_or_null<MDString>(Flag->getOperand(1)))
4898+
return K->getString() == "Debug Info Version";
4899+
return false;
4900+
});
4901+
if (OpIt != ModFlags->op_end()) {
4902+
const MDOperand &ValOp = (*OpIt)->getOperand(2);
4903+
if (auto *CI = mdconst::dyn_extract_or_null<ConstantInt>(ValOp))
4904+
Version = CI->getZExtValue();
4905+
}
4906+
}
4907+
48904908
if (Version == DEBUG_METADATA_VERSION) {
48914909
bool BrokenDebugInfo = false;
48924910
if (verifyModule(M, &llvm::errs(), &BrokenDebugInfo))

llvm/lib/IR/Module.cpp

Lines changed: 15 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -259,10 +259,8 @@ GlobalIFunc *Module::getNamedIFunc(StringRef Name) const {
259259
/// getNamedMetadata - Return the first NamedMDNode in the module with the
260260
/// specified name. This method returns null if a NamedMDNode with the
261261
/// specified name is not found.
262-
NamedMDNode *Module::getNamedMetadata(const Twine &Name) const {
263-
SmallString<256> NameData;
264-
StringRef NameRef = Name.toStringRef(NameData);
265-
return NamedMDSymTab.lookup(NameRef);
262+
NamedMDNode *Module::getNamedMetadata(StringRef Name) const {
263+
return NamedMDSymTab.lookup(Name);
266264
}
267265

268266
/// getOrInsertNamedMetadata - Return the first named MDNode in the module
@@ -296,46 +294,31 @@ bool Module::isValidModFlagBehavior(Metadata *MD, ModFlagBehavior &MFB) {
296294
return false;
297295
}
298296

299-
bool Module::isValidModuleFlag(const MDNode &ModFlag, ModFlagBehavior &MFB,
300-
MDString *&Key, Metadata *&Val) {
301-
if (ModFlag.getNumOperands() < 3)
302-
return false;
303-
if (!isValidModFlagBehavior(ModFlag.getOperand(0), MFB))
304-
return false;
305-
MDString *K = dyn_cast_or_null<MDString>(ModFlag.getOperand(1));
306-
if (!K)
307-
return false;
308-
Key = K;
309-
Val = ModFlag.getOperand(2);
310-
return true;
311-
}
312-
313297
/// getModuleFlagsMetadata - Returns the module flags in the provided vector.
314298
void Module::
315299
getModuleFlagsMetadata(SmallVectorImpl<ModuleFlagEntry> &Flags) const {
316300
const NamedMDNode *ModFlags = getModuleFlagsMetadata();
317301
if (!ModFlags) return;
318302

319303
for (const MDNode *Flag : ModFlags->operands()) {
320-
ModFlagBehavior MFB;
321-
MDString *Key = nullptr;
322-
Metadata *Val = nullptr;
323-
if (isValidModuleFlag(*Flag, MFB, Key, Val)) {
324-
// Check the operands of the MDNode before accessing the operands.
325-
// The verifier will actually catch these failures.
326-
Flags.push_back(ModuleFlagEntry(MFB, Key, Val));
327-
}
304+
// The verifier will catch errors, so no need to check them here.
305+
auto *MFBConstant = mdconst::extract<ConstantInt>(Flag->getOperand(0));
306+
auto MFB = static_cast<ModFlagBehavior>(MFBConstant->getLimitedValue());
307+
MDString *Key = cast<MDString>(Flag->getOperand(1));
308+
Metadata *Val = Flag->getOperand(2);
309+
Flags.push_back(ModuleFlagEntry(MFB, Key, Val));
328310
}
329311
}
330312

331313
/// Return the corresponding value if Key appears in module flags, otherwise
332314
/// return null.
333315
Metadata *Module::getModuleFlag(StringRef Key) const {
334-
SmallVector<Module::ModuleFlagEntry, 8> ModuleFlags;
335-
getModuleFlagsMetadata(ModuleFlags);
336-
for (const ModuleFlagEntry &MFE : ModuleFlags) {
337-
if (Key == MFE.Key->getString())
338-
return MFE.Val;
316+
const NamedMDNode *ModFlags = getModuleFlagsMetadata();
317+
if (!ModFlags)
318+
return nullptr;
319+
for (const MDNode *Flag : ModFlags->operands()) {
320+
if (Key == cast<MDString>(Flag->getOperand(1))->getString())
321+
return Flag->getOperand(2);
339322
}
340323
return nullptr;
341324
}
@@ -388,10 +371,7 @@ void Module::setModuleFlag(ModFlagBehavior Behavior, StringRef Key,
388371
NamedMDNode *ModFlags = getOrInsertModuleFlagsMetadata();
389372
// Replace the flag if it already exists.
390373
for (MDNode *Flag : ModFlags->operands()) {
391-
ModFlagBehavior MFB;
392-
MDString *K = nullptr;
393-
Metadata *V = nullptr;
394-
if (isValidModuleFlag(*Flag, MFB, K, V) && K->getString() == Key) {
374+
if (cast<MDString>(Flag->getOperand(1))->getString() == Key) {
395375
Flag->replaceOperandWith(2, Val);
396376
return;
397377
}

0 commit comments

Comments
 (0)