IR: Add verifier plugins for intrinsic verification #159415
Open
+851
−400
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
Move target-specific logic into target code. The main motivation is that target intrinsics can be very complex. Verifying them properly can benefit from using target-specific infrastructure that is not available in the core library that contains the verifier. Verifying target intrinsics via a "plugin" solves this issue. This does mean that full target-specific verification only happens when the target in question was compiled and initialized. One slightly unfortunate side effect is that llvm-as needs to link against targets in order to fully verify the parsed IR assembly. This shouldn't be a real problem due to dynamic linking, so it seems like a reasonable compromise. I considered the alternative of adding a hook into TargetTransformInfo, as that is how a similar refactoring was done for InstCombine. However, the verifier is invoked in many places e.g. via llvm::verifyModule where TargetTransformInfo may not be readily available.
|
@llvm/pr-subscribers-llvm-ir @llvm/pr-subscribers-backend-arm Author: Nicolai Hähnle (nhaehnle) ChangesMove target-specific logic into target code. The main motivation is that target intrinsics can be very complex. Verifying them properly can benefit from using target-specific infrastructure that is not available in the core library that contains the verifier. Verifying target intrinsics via a "plugin" solves this issue. This does mean that full target-specific verification only happens when the target in question was compiled and initialized. One slightly unfortunate side effect is that llvm-as needs to link against targets in order to fully verify the parsed IR assembly. This shouldn't be a real problem due to dynamic linking, so it seems like a reasonable compromise. I considered the alternative of adding a hook into TargetTransformInfo, as that is how a similar refactoring was done for InstCombine. However, the verifier is invoked in many places e.g. via llvm::verifyModule where TargetTransformInfo may not be readily available. Patch is 54.22 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/159415.diff 20 Files Affected:
diff --git a/llvm/include/llvm/IR/Verifier.h b/llvm/include/llvm/IR/Verifier.h
index 8dbb9c8a41d7e..135c6ab4ebb1f 100644
--- a/llvm/include/llvm/IR/Verifier.h
+++ b/llvm/include/llvm/IR/Verifier.h
@@ -21,20 +21,135 @@
#define LLVM_IR_VERIFIER_H
#include "llvm/ADT/DenseMap.h"
+#include "llvm/IR/DebugProgramInstruction.h"
+#include "llvm/IR/Metadata.h"
+#include "llvm/IR/ModuleSlotTracker.h"
#include "llvm/IR/PassManager.h"
#include "llvm/Support/Compiler.h"
+#include "llvm/Support/Printable.h"
#include <utility>
namespace llvm {
class APInt;
+class Attribute;
+class AttributeList;
+class AttributeSet;
+class CallBase;
+class Comdat;
+class DataLayout;
class Function;
class FunctionPass;
class Instruction;
-class MDNode;
+class LLVMContext;
class Module;
+class Triple;
+class VerifierSupport;
class raw_ostream;
-struct VerifierSupport;
+
+/// Base class for IR verifier plugins.
+///
+/// To add a plugin, derive from this class and then instantiate it once.
+class VerifierPlugin {
+public:
+ VerifierPlugin();
+ virtual ~VerifierPlugin();
+
+ /// Called when the verifier finds a call (or invoke) to an intrinsic it
+ /// doesn't understand.
+ ///
+ /// If the plugin recognizes the intrinsic, it should report any verifier
+ /// errors via the given helper object.
+ virtual void verifyIntrinsicCall(CallBase &Call, VerifierSupport &VS) const;
+};
+
+class VerifierSupport {
+public:
+ raw_ostream *OS;
+ const Module &M;
+ ModuleSlotTracker MST;
+ const Triple &TT;
+ const DataLayout &DL;
+ LLVMContext &Context;
+
+ /// Track the brokenness of the module while recursively visiting.
+ bool Broken = false;
+ /// Broken debug info can be "recovered" from by stripping the debug info.
+ bool BrokenDebugInfo = false;
+ /// Whether to treat broken debug info as an error.
+ bool TreatBrokenDebugInfoAsError = true;
+
+ explicit VerifierSupport(raw_ostream *OS, const Module &M);
+
+private:
+ LLVM_ABI void Write(const Module *M);
+ LLVM_ABI void Write(const Value *V);
+ LLVM_ABI void Write(const Value &V);
+ LLVM_ABI void Write(const DbgRecord *DR);
+ LLVM_ABI void Write(DbgVariableRecord::LocationType Type);
+ LLVM_ABI void Write(const Metadata *MD);
+
+ template <class T> void Write(const MDTupleTypedArrayWrapper<T> &MD) {
+ Write(MD.get());
+ }
+
+ LLVM_ABI void Write(const NamedMDNode *NMD);
+ LLVM_ABI void Write(Type *T);
+ LLVM_ABI void Write(const Comdat *C);
+ LLVM_ABI void Write(const APInt *AI);
+ LLVM_ABI void Write(const unsigned i) { *OS << i << '\n'; }
+
+ // NOLINTNEXTLINE(readability-identifier-naming)
+ LLVM_ABI void Write(const Attribute *A);
+ // NOLINTNEXTLINE(readability-identifier-naming)
+ LLVM_ABI void Write(const AttributeSet *AS);
+ // NOLINTNEXTLINE(readability-identifier-naming)
+ LLVM_ABI void Write(const AttributeList *AL);
+ LLVM_ABI void Write(Printable P) { *OS << P << '\n'; }
+
+ template <typename T> void Write(ArrayRef<T> Vs) {
+ for (const T &V : Vs)
+ Write(V);
+ }
+
+ template <typename T1, typename... Ts>
+ void WriteTs(const T1 &V1, const Ts &...Vs) {
+ Write(V1);
+ WriteTs(Vs...);
+ }
+
+ template <typename... Ts> void WriteTs() {}
+
+public:
+ /// A check failed, so printout out the condition and the message.
+ ///
+ /// This provides a nice place to put a breakpoint if you want to see why
+ /// something is not correct.
+ LLVM_ABI void CheckFailed(const Twine &Message);
+
+ /// A check failed (with values to print).
+ ///
+ /// This calls the Message-only version so that the above is easier to set a
+ /// breakpoint on.
+ template <typename T1, typename... Ts>
+ void CheckFailed(const Twine &Message, const T1 &V1, const Ts &...Vs) {
+ CheckFailed(Message);
+ if (OS)
+ WriteTs(V1, Vs...);
+ }
+
+ /// A debug info check failed.
+ LLVM_ABI void DebugInfoCheckFailed(const Twine &Message);
+
+ /// A debug info check failed (with values to print).
+ template <typename T1, typename... Ts>
+ void DebugInfoCheckFailed(const Twine &Message, const T1 &V1,
+ const Ts &...Vs) {
+ DebugInfoCheckFailed(Message);
+ if (OS)
+ WriteTs(V1, Vs...);
+ }
+};
/// Verify that the TBAA Metadatas are valid.
class TBAAVerifier {
diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index c06b60fd2d9a9..0b393f39e75ff 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -119,6 +119,7 @@
#include "llvm/Support/ErrorHandling.h"
#include "llvm/Support/MathExtras.h"
#include "llvm/Support/ModRef.h"
+#include "llvm/Support/Mutex.h"
#include "llvm/Support/TimeProfiler.h"
#include "llvm/Support/raw_ostream.h"
#include <algorithm>
@@ -127,6 +128,7 @@
#include <memory>
#include <optional>
#include <string>
+#include <thread>
#include <utility>
using namespace llvm;
@@ -136,194 +138,307 @@ static cl::opt<bool> VerifyNoAliasScopeDomination(
cl::desc("Ensure that llvm.experimental.noalias.scope.decl for identical "
"scopes are not dominating"));
-namespace llvm {
+namespace {
-struct VerifierSupport {
- raw_ostream *OS;
- const Module &M;
- ModuleSlotTracker MST;
- const Triple &TT;
- const DataLayout &DL;
- LLVMContext &Context;
+class PluginRegistryLock;
+class PluginRegistryReader;
- /// Track the brokenness of the module while recursively visiting.
- bool Broken = false;
- /// Broken debug info can be "recovered" from by stripping the debug info.
- bool BrokenDebugInfo = false;
- /// Whether to treat broken debug info as an error.
- bool TreatBrokenDebugInfoAsError = true;
+/// Registry for verifier plugins.
+///
+/// The registry satifies the following implementation constraints:
+///
+/// * Support dynamically loading and unloading plugins from a thread (e.g.
+/// from dlopen()/dlclose()) while another thread may be in the verifier
+/// * Fast path for iterating over plugins that is lock-free and avoids
+/// cache-line ping pong
+/// * Plugin teardown may happen due to report_fatal_error from a thread that
+/// is currently in the verifier
+///
+/// The implementation achieves this by registering a "reader" object while the
+/// verifier is active. The reader object holds a hazard pointer to the plugins
+/// list that it is currently using.
+class PluginRegistry {
+ friend PluginRegistryReader;
+ friend PluginRegistryLock;
- explicit VerifierSupport(raw_ostream *OS, const Module &M)
- : OS(OS), M(M), MST(&M), TT(M.getTargetTriple()), DL(M.getDataLayout()),
- Context(M.getContext()) {}
+ using List = SmallVector<const VerifierPlugin *>;
-private:
- void Write(const Module *M) {
- *OS << "; ModuleID = '" << M->getModuleIdentifier() << "'\n";
- }
+ sys::Mutex Mutex;
+ SmallVector<PluginRegistryReader *> Readers;
+ List PluginsStorage[2];
+ std::atomic<List *> Plugins;
- void Write(const Value *V) {
- if (V)
- Write(*V);
+ PluginRegistry() {
+ Plugins.store(&PluginsStorage[0], std::memory_order_relaxed);
}
- void Write(const Value &V) {
- if (isa<Instruction>(V)) {
- V.print(*OS, MST);
- *OS << '\n';
- } else {
- V.printAsOperand(*OS, true, MST);
- *OS << '\n';
- }
- }
+ template <typename FnT> void updatePlugins(FnT &&F);
- void Write(const DbgRecord *DR) {
- if (DR) {
- DR->print(*OS, MST, false);
- *OS << '\n';
- }
+public:
+ static PluginRegistry &get() {
+ static PluginRegistry R;
+ return R;
}
- void Write(DbgVariableRecord::LocationType Type) {
- switch (Type) {
- case DbgVariableRecord::LocationType::Value:
- *OS << "value";
- break;
- case DbgVariableRecord::LocationType::Declare:
- *OS << "declare";
- break;
- case DbgVariableRecord::LocationType::Assign:
- *OS << "assign";
- break;
- case DbgVariableRecord::LocationType::End:
- *OS << "end";
- break;
- case DbgVariableRecord::LocationType::Any:
- *OS << "any";
- break;
- };
+ void addPlugin(const VerifierPlugin *P) {
+ updatePlugins([&](List &Plugins) { Plugins.push_back(P); });
}
- void Write(const Metadata *MD) {
- if (!MD)
- return;
- MD->print(*OS, MST, &M);
- *OS << '\n';
+ void removePlugin(const VerifierPlugin *P) {
+ updatePlugins([&](List &Plugins) {
+ Plugins.erase(std::remove(Plugins.begin(), Plugins.end(), P),
+ Plugins.end());
+ });
}
- template <class T> void Write(const MDTupleTypedArrayWrapper<T> &MD) {
- Write(MD.get());
+ void addReader(PluginRegistryReader *R) {
+ if (llvm_is_multithreaded()) {
+ sys::ScopedLock Lock(Mutex);
+ Readers.push_back(R);
+ }
}
- void Write(const NamedMDNode *NMD) {
- if (!NMD)
- return;
- NMD->print(*OS, MST);
- *OS << '\n';
+ void removeReader(PluginRegistryReader *R) {
+ if (llvm_is_multithreaded()) {
+ sys::ScopedLock Lock(Mutex);
+ Readers.erase(std::remove(Readers.begin(), Readers.end(), R),
+ Readers.end());
+ }
}
+};
- void Write(Type *T) {
- if (!T)
- return;
- *OS << ' ' << *T;
- }
+class PluginRegistryReader {
+ friend PluginRegistry;
+ friend PluginRegistryLock;
- void Write(const Comdat *C) {
- if (!C)
- return;
- *OS << *C;
- }
+ std::atomic<PluginRegistry::List *> HazardPtr;
- void Write(const APInt *AI) {
- if (!AI)
- return;
- *OS << *AI << '\n';
+public:
+ PluginRegistryReader() {
+ HazardPtr.store(nullptr, std::memory_order_relaxed);
+ PluginRegistry::get().addReader(this);
}
- void Write(const unsigned i) { *OS << i << '\n'; }
+ ~PluginRegistryReader() { PluginRegistry::get().removeReader(this); }
+};
- // NOLINTNEXTLINE(readability-identifier-naming)
- void Write(const Attribute *A) {
- if (!A)
- return;
- *OS << A->getAsString() << '\n';
- }
+// Thread-safe update of the plugins list. Take the lock, copy & update the
+// list, then wait for all readers to let go of the old version of the list
+// before releasing the lock.
+template <typename FnT> void PluginRegistry::updatePlugins(FnT &&F) {
+ if (llvm_is_multithreaded()) {
+ sys::ScopedLock Lock(Mutex);
+
+ List *OldList = Plugins.load(std::memory_order_relaxed);
+ List *NewList = (OldList == &PluginsStorage[0]) ? &PluginsStorage[1]
+ : &PluginsStorage[0];
+
+ // We're about to write to NewList. Spin wait to ensure no reader is
+ // accessing it.
+ for (auto *R : Readers) {
+ while (R->HazardPtr.load(std::memory_order_seq_cst) == NewList) {
+ // Let's yield to avoid a pathological busy wait. This really should
+ // only happen in the corner case where multiple users of LLVM exist
+ // in the same process and are initialized or torn down concurrently,
+ // so don't sweat the details.
+ std::this_thread::yield();
+ }
+ }
- // NOLINTNEXTLINE(readability-identifier-naming)
- void Write(const AttributeSet *AS) {
- if (!AS)
- return;
- *OS << AS->getAsString() << '\n';
+ *NewList = *OldList;
+ F(*NewList);
+
+ Plugins.store(NewList, std::memory_order_seq_cst);
+ } else {
+ // Avoid unnecessary copies when compiling without multi-threading
+ // support.
+ F(*Plugins.load(std::memory_order_relaxed));
}
+}
- // NOLINTNEXTLINE(readability-identifier-naming)
- void Write(const AttributeList *AL) {
- if (!AL)
- return;
- AL->print(*OS);
+class PluginRegistryLock {
+ PluginRegistryLock(PluginRegistryLock &) = delete;
+ PluginRegistryLock(PluginRegistryLock &&) = delete;
+ PluginRegistryLock &operator=(PluginRegistryLock &) = delete;
+ PluginRegistryLock &operator=(PluginRegistryLock &&) = delete;
+
+ PluginRegistryReader &Reader;
+
+public:
+ explicit PluginRegistryLock(PluginRegistryReader &Reader) : Reader(Reader) {
+ assert(!Reader.HazardPtr &&
+ "cannot have multiple PluginRegistryLocks through the same reader");
+
+ auto &Registry = PluginRegistry::get();
+
+ // The memory order of the initial load is irrelevant since we re-check the
+ // pointer using a sequentially consistent load later.
+ PluginRegistry::List *L = Registry.Plugins.load(std::memory_order_relaxed);
+
+ if (llvm_is_multithreaded()) {
+ for (;;) {
+ Reader.HazardPtr.store(L, std::memory_order_seq_cst);
+
+ PluginRegistry::List *Check =
+ Registry.Plugins.load(std::memory_order_seq_cst);
+ if (Check == L)
+ break;
+
+ L = Check;
+ }
+ } else {
+ Reader.HazardPtr.store(L, std::memory_order_relaxed);
+ }
}
- void Write(Printable P) { *OS << P << '\n'; }
+ ~PluginRegistryLock() {
+ assert(Reader.HazardPtr);
- template <typename T> void Write(ArrayRef<T> Vs) {
- for (const T &V : Vs)
- Write(V);
+ if (llvm_is_multithreaded()) {
+ Reader.HazardPtr.store(nullptr, std::memory_order_seq_cst);
+ } else {
+ Reader.HazardPtr.store(nullptr, std::memory_order_relaxed);
+ }
}
- template <typename T1, typename... Ts>
- void WriteTs(const T1 &V1, const Ts &... Vs) {
- Write(V1);
- WriteTs(Vs...);
+ ArrayRef<const VerifierPlugin *> get() const {
+ return *Reader.HazardPtr.load(std::memory_order_relaxed);
}
+};
- template <typename... Ts> void WriteTs() {}
+} // anonymous namespace
-public:
- /// A check failed, so printout out the condition and the message.
- ///
- /// This provides a nice place to put a breakpoint if you want to see why
- /// something is not correct.
- void CheckFailed(const Twine &Message) {
- if (OS)
- *OS << Message << '\n';
- Broken = true;
+VerifierPlugin::VerifierPlugin() { PluginRegistry::get().addPlugin(this); }
+
+VerifierPlugin::~VerifierPlugin() { PluginRegistry::get().removePlugin(this); }
+
+void VerifierPlugin::verifyIntrinsicCall(CallBase &Call,
+ VerifierSupport &VS) const {}
+
+VerifierSupport::VerifierSupport(raw_ostream *OS, const Module &M)
+ : OS(OS), M(M), MST(&M), TT(M.getTargetTriple()), DL(M.getDataLayout()),
+ Context(M.getContext()) {}
+
+void VerifierSupport::Write(const Module *M) {
+ *OS << "; ModuleID = '" << M->getModuleIdentifier() << "'\n";
+}
+
+void VerifierSupport::Write(const Value *V) {
+ if (V)
+ Write(*V);
+}
+
+void VerifierSupport::Write(const Value &V) {
+ if (isa<Instruction>(V)) {
+ V.print(*OS, MST);
+ *OS << '\n';
+ } else {
+ V.printAsOperand(*OS, true, MST);
+ *OS << '\n';
}
+}
- /// A check failed (with values to print).
- ///
- /// This calls the Message-only version so that the above is easier to set a
- /// breakpoint on.
- template <typename T1, typename... Ts>
- void CheckFailed(const Twine &Message, const T1 &V1, const Ts &... Vs) {
- CheckFailed(Message);
- if (OS)
- WriteTs(V1, Vs...);
- }
-
- /// A debug info check failed.
- void DebugInfoCheckFailed(const Twine &Message) {
- if (OS)
- *OS << Message << '\n';
- Broken |= TreatBrokenDebugInfoAsError;
- BrokenDebugInfo = true;
- }
-
- /// A debug info check failed (with values to print).
- template <typename T1, typename... Ts>
- void DebugInfoCheckFailed(const Twine &Message, const T1 &V1,
- const Ts &... Vs) {
- DebugInfoCheckFailed(Message);
- if (OS)
- WriteTs(V1, Vs...);
+void VerifierSupport::Write(const DbgRecord *DR) {
+ if (DR) {
+ DR->print(*OS, MST, false);
+ *OS << '\n';
}
-};
+}
+
+void VerifierSupport::Write(DbgVariableRecord::LocationType Type) {
+ switch (Type) {
+ case DbgVariableRecord::LocationType::Value:
+ *OS << "value";
+ break;
+ case DbgVariableRecord::LocationType::Declare:
+ *OS << "declare";
+ break;
+ case DbgVariableRecord::LocationType::Assign:
+ *OS << "assign";
+ break;
+ case DbgVariableRecord::LocationType::End:
+ *OS << "end";
+ break;
+ case DbgVariableRecord::LocationType::Any:
+ *OS << "any";
+ break;
+ };
+}
-} // namespace llvm
+void VerifierSupport::Write(const Metadata *MD) {
+ if (!MD)
+ return;
+ MD->print(*OS, MST, &M);
+ *OS << '\n';
+}
+
+void VerifierSupport::Write(const NamedMDNode *NMD) {
+ if (!NMD)
+ return;
+ NMD->print(*OS, MST);
+ *OS << '\n';
+}
+
+void VerifierSupport::Write(Type *T) {
+ if (!T)
+ return;
+ *OS << ' ' << *T;
+}
+
+void VerifierSupport::Write(const Comdat *C) {
+ if (!C)
+ return;
+ *OS << *C;
+}
+
+void VerifierSupport::Write(const APInt *AI) {
+ if (!AI)
+ return;
+ *OS << *AI << '\n';
+}
+
+// NOLINTNEXTLINE(readability-identifier-naming)
+void VerifierSupport::Write(const Attribute *A) {
+ if (!A)
+ return;
+ *OS << A->getAsString() << '\n';
+}
+
+// NOLINTNEXTLINE(readability-identifier-naming)
+void VerifierSupport::Write(const AttributeSet *AS) {
+ if (!AS)
+ return;
+ *OS << AS->getAsString() << '\n';
+}
+
+// NOLINTNEXTLINE(readability-identifier-naming)
+void VerifierSupport::Write(const AttributeList *AL) {
+ if (!AL)
+ return;
+ AL->print(*OS);
+}
+
+void VerifierSupport::CheckFailed(const Twine &Message) {
+ if (OS)
+ *OS << Message << '\n';
+ Broken = true;
+}
+
+/// A debug info check failed.
+void VerifierSupport::DebugInfoCheckFailed(const Twine &Message) {
+ if (OS)
+ *OS << Message << '\n';
+ Broken |= TreatBrokenDebugInfoAsError;
+ BrokenDebugInfo = true;
+}
namespace {
class Verifier : public InstVisitor<Verifier>, VerifierSupport {
friend class InstVisitor<Verifier>;
+
+ PluginRegistryReader PluginsReader;
+
DominatorTree DT;
/// When verifying a basic block, keep track of all of the
@@ -5660,8 +5775,12 @@ void Verifier::visitIntrinsicCall(Intrinsic::ID ID, CallBase &Call) {
}
switch (ID) {
- default:
+ default: {
+ PluginRegistryLock Lock(PluginsReader);
+ for (const VerifierPlugin *P : Lock.get())
+ P->verifyIntrinsicCall(Call, *this);
break;
+ }
case Intrinsic::assume: {
for (auto &Elem : Call.bundle_op_infos()) {
unsigned ArgCount = Elem.End - Elem.Begin;
@@ -6549,37 +6668,12 @@ void Verifier::visitIntrinsicCall(Intrinsic::ID ID, CallBase &Call) {
break;
}
case Intrinsic::preserve_array_access_index:
- case Intrinsic::preserve_struct_access_index:
- case Intrinsic::aarch64_ldaxr:
- case Intrinsic::aarch64_ldxr:
- case Intrinsic::arm_ldaex:
- case Intrinsic::arm_ldrex: {
+ case Intrinsic::preserve_struct_access_index: {
Type *ElemTy = Call.getParamElementType(0);
Check(ElemTy, "Intrinsic requires elementtype attribute on first argument.",
&Call);
break;
}
- case Intrinsic::aarch64_stlxr:
- case Intrinsic::aarch64_stxr:
- case Intrinsic::arm_stlex:
- case Intrinsic::arm_strex: {
- Type *ElemTy = Call.getAttributes().getParamElementType(1);
- Check(ElemTy,
- "Intrinsic requires elementtype attribute on second argument.",
- &Call);
- break;
- }
- case Intrinsic::aarch64_prefetch: {
- Check(cast<ConstantInt>(Call.getArgOperand(1))->getZExtValue() < 2,
- "write argument to llvm.aarch64.prefetch must be 0 or 1", Call);
- Check(cast<ConstantInt>(Call.getArgOperand(2))->getZExtValue() < 4,
- "target argument to llvm.aarch64.prefetch must be 0-3", Call);
- Check(cast<ConstantInt>(Call.getArgOperand(3))->getZExtValue() < 2,
- "stream argument to llvm.aarch64.prefetch must be 0 or 1", Call);
- Check(cast<ConstantInt>(Call.getArgOperand(4))->getZExtValue() < 2,
- "isdata argument to llvm.aarch64.prefetch must be 0 or 1", Call);
- break;
- }
case Intrinsic::callbr_landingpad: {
const auto *CBR = dyn_cast<CallBrInst>(Call.getOperand(0));
Check(CBR, "intrinstic requires callbr operand", &Call);
@@ -6606,232 +6700,6 @@ void Verifier::visitIntrinsicCall(Intrinsic::ID ID, CallBase &Call) {
&Call);
break;
}
- case Intrinsic::amdgcn_cs_chain: {
- auto CallerCC = Call.getCaller()->getCallingConv();
- switch (CallerCC) {
- case CallingConv::AMDGPU_CS:
- case CallingConv::AMDGPU_CS_Chain:
- case CallingConv::AMDGPU_CS_ChainPreserve:
- break;
- default:
- CheckFailed("Intrinsic can only be used from functions with the "
- "amdgpu_cs, amdgpu_cs_chain or amdgpu_cs_chain_preserve "
- "calling conventions",
- &Call);
- break;
- }
-
- Check(Call.param...
[truncated]
|
|
There were some debates on whether we need target dependent verifier in one of @jofrn PR. |
TTI will not work because of circular dependence. |
I think this is quite different from that PR. Target intrinsics are inherently target-specific, so it's not a problem to have target-specific verification for them as well. This is quite different from allowing targets to come up with arbitrary rules for target-independent IR constructs. |
arsenm
reviewed
Sep 18, 2025
|
|
||
| class VerifierSupport { | ||
| public: | ||
| raw_ostream *OS; |
There was a problem hiding this comment.
Can this be a reference? Can the null stream be a substitute for the current null usage?
| }; | ||
|
|
||
| class VerifierSupport { | ||
| public: |
|
|
||
| namespace { | ||
|
|
||
| #define Check(C, ...) \ |
There was a problem hiding this comment.
Does every target really need to define the same macro
Comment on lines
+2
to
+3
| AllTargetsCodeGens | ||
| AllTargetsInfos |
There was a problem hiding this comment.
I think it's important to have llvm-as not depend on complete target builds. I feel most of the value-add of llvm-as/llvm-dis is it's easier to start work on core IR changes without having to rebuild the universe
|
It would be an improvement to just split this out into the separate files without placing it in the backends |
What really motivated this is that I was exploring new intrinsics for which verification could benefit from some shared defines and helper functions, and there really isn't a great place to put those. I thought of putting them into Target/AMDGPU/Utils, which is what motivated this change (where just splitting out into a separate file is insufficient). I see your point about As for what the alternatives are... it seems a lot of stuff just ends up getting dumped into Support (like NVPTXAddrSpace.h or DXILABI.h). Perhaps that stuff should go under IR/${target}/ directories instead? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Move target-specific logic into target code.
The main motivation is that target intrinsics can be very complex. Verifying them properly can benefit from using target-specific infrastructure that is not available in the core library that contains the verifier. Verifying target intrinsics via a "plugin" solves this issue.
This does mean that full target-specific verification only happens when the target in question was compiled and initialized. One slightly unfortunate side effect is that llvm-as needs to link against targets in order to fully verify the parsed IR assembly. This shouldn't be a real problem due to dynamic linking, so it seems like a reasonable compromise.
I considered the alternative of adding a hook into TargetTransformInfo, as that is how a similar refactoring was done for InstCombine. However, the verifier is invoked in many places e.g. via llvm::verifyModule where TargetTransformInfo may not be readily available.