Skip to content

[TargetVerifier][AMDGPU] Add TargetVerifier. #123609

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

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open

Conversation

jofrn
Copy link
Contributor

@jofrn jofrn commented Jan 20, 2025

This pass verifies the IR for an individual backend. This is different than Lint because it consolidates all checks for a given backend in a single pass. A check for Lint may be undefined behavior across all targets, whereas a check in TargetVerifier would only pertain to the specified target but can check more than just undefined behavior such are IR validity. A use case of this would be to reject programs with invalid IR while fuzzing.

Copy link

github-actions bot commented Jan 20, 2025

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff HEAD~1 HEAD --extensions h,cpp -- llvm/include/llvm/Target/TargetVerifier.h llvm/include/llvm/Target/TargetVerify/AMDGPUTargetVerifier.h llvm/lib/Target/AMDGPU/AMDGPUTargetVerifier.cpp llvm/lib/Target/AMDGPU/AMDGPU.h llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
View the diff from clang-format here.
diff --git a/llvm/include/llvm/Target/TargetVerifier.h b/llvm/include/llvm/Target/TargetVerifier.h
index 1d12eb55b..49f9a252f 100644
--- a/llvm/include/llvm/Target/TargetVerifier.h
+++ b/llvm/include/llvm/Target/TargetVerifier.h
@@ -1,4 +1,5 @@
-//===-- llvm/Target/TargetVerifier.h - LLVM IR Target Verifier ---*- C++ -*-===//
+//===-- llvm/Target/TargetVerifier.h - LLVM IR Target Verifier ---*- C++
+//-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
@@ -20,8 +21,8 @@
 #ifndef LLVM_TARGET_VERIFIER_H
 #define LLVM_TARGET_VERIFIER_H
 
-#include "llvm/IR/PassManager.h"
 #include "llvm/IR/Module.h"
+#include "llvm/IR/PassManager.h"
 #include "llvm/TargetParser/Triple.h"
 
 namespace llvm {
@@ -59,10 +60,11 @@ protected:
   /// 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) {
+  void CheckFailed(const Twine &Message, const T1 &V1, const Ts &...Vs) {
     CheckFailed(Message);
     WriteValues({V1, Vs...});
   }
+
 public:
   Module *Mod;
   Triple TT;
@@ -73,8 +75,7 @@ public:
   bool IsValid = true;
 
   TargetVerify(Module *Mod)
-      : Mod(Mod), TT(Mod->getTargetTriple()),
-        MessagesStr(Messages) {}
+      : Mod(Mod), TT(Mod->getTargetTriple()), MessagesStr(Messages) {}
 
   virtual bool run(Function &F) = 0;
 };
diff --git a/llvm/include/llvm/Target/TargetVerify/AMDGPUTargetVerifier.h b/llvm/include/llvm/Target/TargetVerify/AMDGPUTargetVerifier.h
index 49bcbc884..b5795b77e 100644
--- a/llvm/include/llvm/Target/TargetVerify/AMDGPUTargetVerifier.h
+++ b/llvm/include/llvm/Target/TargetVerify/AMDGPUTargetVerifier.h
@@ -1,8 +1,9 @@
-//===-- llvm/Target/TargetVerify/AMDGPUTargetVerifier.h - AMDGPU ---*- C++ -*-===//
+//===-- llvm/Target/TargetVerify/AMDGPUTargetVerifier.h - AMDGPU ---*- C++
+//-*-===//
 ////
-//// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-//// See https://llvm.org/LICENSE.txt for license information.
-//// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//// Part of the LLVM Project, under the Apache License v2.0 with LLVM
+///Exceptions. / See https://llvm.org/LICENSE.txt for license information. /
+///SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 ////
 ////===----------------------------------------------------------------------===//
 ////
@@ -22,8 +23,8 @@
 
 #include "llvm/Target/TargetVerifier.h"
 
-#include "llvm/Analysis/UniformityAnalysis.h"
 #include "llvm/Analysis/PostDominators.h"
+#include "llvm/Analysis/UniformityAnalysis.h"
 #include "llvm/IR/Dominators.h"
 
 namespace llvm {
@@ -43,11 +44,11 @@ public:
   PostDominatorTree *PDT = nullptr;
   UniformityInfo *UA = nullptr;
 
-  AMDGPUTargetVerify(Module *Mod)
-    : TargetVerify(Mod), Mod(Mod) {}
+  AMDGPUTargetVerify(Module *Mod) : TargetVerify(Mod), Mod(Mod) {}
 
-  AMDGPUTargetVerify(Module *Mod, DominatorTree *DT, PostDominatorTree *PDT, UniformityInfo *UA)
-    : TargetVerify(Mod), Mod(Mod), DT(DT), PDT(PDT), UA(UA) {}
+  AMDGPUTargetVerify(Module *Mod, DominatorTree *DT, PostDominatorTree *PDT,
+                     UniformityInfo *UA)
+      : TargetVerify(Mod), Mod(Mod), DT(DT), PDT(PDT), UA(UA) {}
 
   bool run(Function &F) override;
 };
diff --git a/llvm/lib/Target/AMDGPU/AMDGPUTargetVerifier.cpp b/llvm/lib/Target/AMDGPU/AMDGPUTargetVerifier.cpp
index 2ca0bbeb5..889f6b6d6 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUTargetVerifier.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPUTargetVerifier.cpp
@@ -1,8 +1,9 @@
-//===-- AMDGPUTargetVerifier.cpp - AMDGPU -------------------------*- C++ -*-===//
+//===-- AMDGPUTargetVerifier.cpp - AMDGPU -------------------------*- C++
+//-*-===//
 ////
-//// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-//// See https://llvm.org/LICENSE.txt for license information.
-//// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//// Part of the LLVM Project, under the Apache License v2.0 with LLVM
+///Exceptions. / See https://llvm.org/LICENSE.txt for license information. /
+///SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 ////
 ////===----------------------------------------------------------------------===//
 ////
@@ -17,18 +18,18 @@
 ////
 ////===----------------------------------------------------------------------===//
 
-#include "AMDGPU.h"
 #include "llvm/Target/TargetVerify/AMDGPUTargetVerifier.h"
+#include "AMDGPU.h"
 
-#include "llvm/Analysis/UniformityAnalysis.h"
 #include "llvm/Analysis/PostDominators.h"
-#include "llvm/Support/Debug.h"
+#include "llvm/Analysis/UniformityAnalysis.h"
 #include "llvm/IR/Dominators.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/IntrinsicsAMDGPU.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/Value.h"
+#include "llvm/Support/Debug.h"
 
 #include "llvm/Support/raw_ostream.h"
 
@@ -39,53 +40,53 @@ using namespace llvm;
   do {                                                                         \
     if (!(C)) {                                                                \
       TargetVerify::CheckFailed(__VA_ARGS__);                                  \
-      return false;                                                                  \
+      return false;                                                            \
     }                                                                          \
   } while (false)
 
 namespace llvm {
 
 static bool isShader(CallingConv::ID CC) {
-  switch(CC) {
-    case CallingConv::AMDGPU_VS:
-    case CallingConv::AMDGPU_LS:
-    case CallingConv::AMDGPU_HS:
-    case CallingConv::AMDGPU_ES:
-    case CallingConv::AMDGPU_GS:
-    case CallingConv::AMDGPU_PS:
-    case CallingConv::AMDGPU_CS_Chain:
-    case CallingConv::AMDGPU_CS_ChainPreserve:
-    case CallingConv::AMDGPU_CS:
-      return true;
-    default:
-      return false;
+  switch (CC) {
+  case CallingConv::AMDGPU_VS:
+  case CallingConv::AMDGPU_LS:
+  case CallingConv::AMDGPU_HS:
+  case CallingConv::AMDGPU_ES:
+  case CallingConv::AMDGPU_GS:
+  case CallingConv::AMDGPU_PS:
+  case CallingConv::AMDGPU_CS_Chain:
+  case CallingConv::AMDGPU_CS_ChainPreserve:
+  case CallingConv::AMDGPU_CS:
+    return true;
+  default:
+    return false;
   }
 }
 
 bool AMDGPUTargetVerify::run(Function &F) {
   // Ensure shader calling convention returns void
   if (isShader(F.getCallingConv()))
-    Check(F.getReturnType() == Type::getVoidTy(F.getContext()), "Shaders must return void");
+    Check(F.getReturnType() == Type::getVoidTy(F.getContext()),
+          "Shaders must return void");
 
   for (auto &BB : F) {
 
     for (auto &I : BB) {
 
-      if (auto *CI = dyn_cast<CallInst>(&I))
-      {
+      if (auto *CI = dyn_cast<CallInst>(&I)) {
         // Ensure no kernel to kernel calls.
         CallingConv::ID CalleeCC = CI->getCallingConv();
-        if (CalleeCC == CallingConv::AMDGPU_KERNEL)
-        {
-          CallingConv::ID CallerCC = CI->getParent()->getParent()->getCallingConv();
+        if (CalleeCC == CallingConv::AMDGPU_KERNEL) {
+          CallingConv::ID CallerCC =
+              CI->getParent()->getParent()->getCallingConv();
           Check(CallerCC != CallingConv::AMDGPU_KERNEL,
-            "A kernel may not call a kernel", CI->getParent()->getParent());
+                "A kernel may not call a kernel", CI->getParent()->getParent());
         }
 
         // Ensure chain intrinsics are followed by unreachables.
         if (CI->getIntrinsicID() == Intrinsic::amdgcn_cs_chain)
           Check(isa_and_present<UnreachableInst>(CI->getNextNode()),
-            "llvm.amdgcn.cs.chain must be followed by unreachable", CI);
+                "llvm.amdgcn.cs.chain must be followed by unreachable", CI);
       }
     }
   }
@@ -98,7 +99,8 @@ bool AMDGPUTargetVerify::run(Function &F) {
   return true;
 }
 
-PreservedAnalyses AMDGPUTargetVerifierPass::run(Function &F, FunctionAnalysisManager &AM) {
+PreservedAnalyses AMDGPUTargetVerifierPass::run(Function &F,
+                                                FunctionAnalysisManager &AM) {
   auto *Mod = F.getParent();
 
   auto UA = &AM.getResult<UniformityInfoAnalysis>(F);
@@ -122,9 +124,10 @@ struct AMDGPUTargetVerifierLegacyPass : public FunctionPass {
   std::unique_ptr<AMDGPUTargetVerify> TV;
   bool FatalErrors = false;
 
-  AMDGPUTargetVerifierLegacyPass(bool FatalErrors) : FunctionPass(ID),
-    FatalErrors(FatalErrors) {
-    initializeAMDGPUTargetVerifierLegacyPassPass(*PassRegistry::getPassRegistry());
+  AMDGPUTargetVerifierLegacyPass(bool FatalErrors)
+      : FunctionPass(ID), FatalErrors(FatalErrors) {
+    initializeAMDGPUTargetVerifierLegacyPassPass(
+        *PassRegistry::getPassRegistry());
   }
 
   bool doInitialization(Module &M) override {
@@ -167,4 +170,5 @@ FunctionPass *createAMDGPUTargetVerifierLegacyPass(bool FatalErrors) {
   return new AMDGPUTargetVerifierLegacyPass(FatalErrors);
 }
 } // namespace llvm
-INITIALIZE_PASS(AMDGPUTargetVerifierLegacyPass, "amdgpu-tgtverifier", "AMDGPU Target Verifier", false, false)
+INITIALIZE_PASS(AMDGPUTargetVerifierLegacyPass, "amdgpu-tgtverifier",
+                "AMDGPU Target Verifier", false, false)

@JonChesterfield
Copy link
Collaborator

Just to say I love this premise. More checks in the verifier is a great way to narrow down which pass has created broken code.

The alloca-must-be-const looks slightly optimistic to me - is that an invariant clang sema enforces for c++ input? I can see how we'd struggle to lower it, just not sure whether it's illegal in all IR or only in some of it. Target verifier should probably be invariants that hold between every pass.

Main thing is it would be really nice for this to be a hook that the usual verifier calls into as opposed to a separate pass, as verify is already in the debugging flow for lots of people. Means these checks can be easily inserted between every IR pass etc without altering infrastructure elsewhere.

@jofrn
Copy link
Contributor Author

jofrn commented Feb 3, 2025

Thanks for the feedback.

The alloca-must-be-const looks slightly optimistic to me - is that an invariant clang sema enforces for c++ input? I can see how we'd struggle to lower it, just not sure whether it's illegal in all IR or only in some of it. Target verifier should probably be invariants that hold between every pass.

Removing the check. I see that it is supported somewhat yet we try to replace it during DAG Selection.

Main thing is it would be really nice for this to be a hook that the usual verifier calls into as opposed to a separate pass, as verify is already in the debugging flow for lots of people. Means these checks can be easily inserted between every IR pass etc without altering infrastructure elsewhere.

Adding the hook... Please let me know what you think.

@jofrn jofrn marked this pull request as ready for review April 2, 2025 12:35
@llvmbot llvmbot added backend:AMDGPU LTO Link time optimization (regular/full LTO or ThinLTO) llvm:ir llvm:analysis labels Apr 2, 2025
@llvmbot
Copy link
Member

llvmbot commented Apr 2, 2025

@llvm/pr-subscribers-clang
@llvm/pr-subscribers-clang-codegen
@llvm/pr-subscribers-llvm-ir
@llvm/pr-subscribers-llvm-analysis

@llvm/pr-subscribers-backend-amdgpu

Author: None (jofrn)

Changes

This pass verifies the IR for an individual backend. This is different than Lint because it consolidates all checks for a given backend in a single pass. A check for Lint may be undefined behavior across all targets, whereas a check in TargetVerifier would only pertain to the specified target but can check more than just undefined behavior such are IR validity. A use case of this would be to reject programs with invalid IR while fuzzing.


Patch is 34.37 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/123609.diff

22 Files Affected:

  • (modified) llvm/include/llvm/IR/Module.h (+4)
  • (modified) llvm/include/llvm/Passes/StandardInstrumentations.h (+4-2)
  • (added) llvm/include/llvm/Target/TargetVerifier.h (+83)
  • (added) llvm/include/llvm/Target/TargetVerify/AMDGPUTargetVerifier.h (+54)
  • (modified) llvm/lib/Analysis/Lint.cpp (+6-3)
  • (modified) llvm/lib/IR/Verifier.cpp (+14-4)
  • (modified) llvm/lib/LTO/LTOBackend.cpp (+1-1)
  • (modified) llvm/lib/LTO/ThinLTOCodeGenerator.cpp (+1-1)
  • (modified) llvm/lib/Passes/CMakeLists.txt (+1)
  • (modified) llvm/lib/Passes/PassBuilderBindings.cpp (+1-1)
  • (modified) llvm/lib/Passes/StandardInstrumentations.cpp (+15-4)
  • (added) llvm/lib/Target/AMDGPU/AMDGPUTargetVerifier.cpp (+213)
  • (modified) llvm/lib/Target/AMDGPU/CMakeLists.txt (+1)
  • (modified) llvm/lib/Target/CMakeLists.txt (+2)
  • (added) llvm/test/CodeGen/AMDGPU/tgt-verify-llc-fail.ll (+6)
  • (added) llvm/test/CodeGen/AMDGPU/tgt-verify-llc-pass.ll (+6)
  • (added) llvm/test/CodeGen/AMDGPU/tgt-verify.ll (+62)
  • (modified) llvm/tools/llc/NewPMDriver.cpp (+1-1)
  • (added) llvm/tools/llvm-tgt-verify/CMakeLists.txt (+34)
  • (added) llvm/tools/llvm-tgt-verify/llvm-tgt-verify.cpp (+172)
  • (modified) llvm/tools/opt/NewPMDriver.cpp (+1-1)
  • (modified) llvm/unittests/IR/PassManagerTest.cpp (+3-3)
diff --git a/llvm/include/llvm/IR/Module.h b/llvm/include/llvm/IR/Module.h
index 12b50fc506516..7844cb216ed57 100644
--- a/llvm/include/llvm/IR/Module.h
+++ b/llvm/include/llvm/IR/Module.h
@@ -211,6 +211,10 @@ class LLVM_ABI Module {
 /// @name Constructors
 /// @{
 public:
+  /// Is this Module valid as determined by one of the verification passes
+  /// i.e. Lint, Verifier, TargetVerifier.
+  bool IsValid = true;
+
   /// Is this Module using intrinsics to record the position of debugging
   /// information, or non-intrinsic records? See IsNewDbgInfoFormat in
   /// \ref BasicBlock.
diff --git a/llvm/include/llvm/Passes/StandardInstrumentations.h b/llvm/include/llvm/Passes/StandardInstrumentations.h
index 4e62ee9c00daf..f794555b7a96d 100644
--- a/llvm/include/llvm/Passes/StandardInstrumentations.h
+++ b/llvm/include/llvm/Passes/StandardInstrumentations.h
@@ -464,7 +464,8 @@ class VerifyInstrumentation {
 public:
   VerifyInstrumentation(bool DebugLogging) : DebugLogging(DebugLogging) {}
   void registerCallbacks(PassInstrumentationCallbacks &PIC,
-                         ModuleAnalysisManager *MAM);
+                         ModuleAnalysisManager *MAM,
+			 FunctionAnalysisManager *FAM);
 };
 
 /// This class implements --time-trace functionality for new pass manager.
@@ -609,7 +610,8 @@ class StandardInstrumentations {
   // Register all the standard instrumentation callbacks. If \p FAM is nullptr
   // then PreservedCFGChecker is not enabled.
   void registerCallbacks(PassInstrumentationCallbacks &PIC,
-                         ModuleAnalysisManager *MAM = nullptr);
+                         ModuleAnalysisManager *MAM,
+			 FunctionAnalysisManager *FAM);
 
   TimePassesHandler &getTimePasses() { return TimePasses; }
 };
diff --git a/llvm/include/llvm/Target/TargetVerifier.h b/llvm/include/llvm/Target/TargetVerifier.h
new file mode 100644
index 0000000000000..ad5aeb895953d
--- /dev/null
+++ b/llvm/include/llvm/Target/TargetVerifier.h
@@ -0,0 +1,83 @@
+//===-- llvm/Target/TargetVerifier.h - LLVM IR Target Verifier ---*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file defines target verifier interfaces that can be used for some
+// validation of input to the system, and for checking that transformations
+// haven't done something bad. In contrast to the Verifier or Lint, the
+// TargetVerifier looks for constructions invalid to a particular target
+// machine.
+//
+// To see what specifically is checked, look at TargetVerifier.cpp or an
+// individual backend's TargetVerifier.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_TARGET_VERIFIER_H
+#define LLVM_TARGET_VERIFIER_H
+
+#include "llvm/IR/PassManager.h"
+#include "llvm/IR/Module.h"
+#include "llvm/TargetParser/Triple.h"
+
+namespace llvm {
+
+class Function;
+
+class TargetVerifierPass : public PassInfoMixin<TargetVerifierPass> {
+public:
+  PreservedAnalyses run(Function &F, FunctionAnalysisManager &AM) {}
+};
+
+class TargetVerify {
+protected:
+  void WriteValues(ArrayRef<const Value *> Vs) {
+    for (const Value *V : Vs) {
+      if (!V)
+        continue;
+      if (isa<Instruction>(V)) {
+        MessagesStr << *V << '\n';
+      } else {
+        V->printAsOperand(MessagesStr, true, Mod);
+        MessagesStr << '\n';
+      }
+    }
+  }
+
+  /// 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) { MessagesStr << Message << '\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);
+    WriteValues({V1, Vs...});
+  }
+public:
+  Module *Mod;
+  Triple TT;
+
+  std::string Messages;
+  raw_string_ostream MessagesStr;
+
+  TargetVerify(Module *Mod)
+      : Mod(Mod), TT(Triple::normalize(Mod->getTargetTriple())),
+        MessagesStr(Messages) {}
+
+  void run(Function &F) {};
+  void run(Function &F, FunctionAnalysisManager &AM);
+};
+
+} // namespace llvm
+
+#endif // LLVM_TARGET_VERIFIER_H
diff --git a/llvm/include/llvm/Target/TargetVerify/AMDGPUTargetVerifier.h b/llvm/include/llvm/Target/TargetVerify/AMDGPUTargetVerifier.h
new file mode 100644
index 0000000000000..d8a3fda4f87dc
--- /dev/null
+++ b/llvm/include/llvm/Target/TargetVerify/AMDGPUTargetVerifier.h
@@ -0,0 +1,54 @@
+//===-- llvm/Target/TargetVerify/AMDGPUTargetVerifier.h - AMDGPU ---*- C++ -*-===//
+////
+//// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+//// See https://llvm.org/LICENSE.txt for license information.
+//// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+////
+////===----------------------------------------------------------------------===//
+////
+//// This file defines target verifier interfaces that can be used for some
+//// validation of input to the system, and for checking that transformations
+//// haven't done something bad. In contrast to the Verifier or Lint, the
+//// TargetVerifier looks for constructions invalid to a particular target
+//// machine.
+////
+//// To see what specifically is checked, look at an individual backend's
+//// TargetVerifier.
+////
+////===----------------------------------------------------------------------===//
+
+#ifndef LLVM_AMDGPU_TARGET_VERIFIER_H
+#define LLVM_AMDGPU_TARGET_VERIFIER_H
+
+#include "llvm/Target/TargetVerifier.h"
+
+#include "llvm/Analysis/UniformityAnalysis.h"
+#include "llvm/Analysis/PostDominators.h"
+#include "llvm/IR/Dominators.h"
+
+namespace llvm {
+
+class Function;
+
+class AMDGPUTargetVerifierPass : public TargetVerifierPass {
+public:
+  PreservedAnalyses run(Function &F, FunctionAnalysisManager &AM);
+};
+
+class AMDGPUTargetVerify : public TargetVerify {
+public:
+  Module *Mod;
+
+  DominatorTree *DT;
+  PostDominatorTree *PDT;
+  UniformityInfo *UA;
+
+  AMDGPUTargetVerify(Module *Mod, DominatorTree *DT, PostDominatorTree *PDT, UniformityInfo *UA)
+    : TargetVerify(Mod), Mod(Mod), DT(DT), PDT(PDT), UA(UA) {}
+
+  void run(Function &F);
+};
+
+} // namespace llvm
+
+#endif // LLVM_AMDGPU_TARGET_VERIFIER_H
diff --git a/llvm/lib/Analysis/Lint.cpp b/llvm/lib/Analysis/Lint.cpp
index e9d96a0c2972a..1a42131b8ed1f 100644
--- a/llvm/lib/Analysis/Lint.cpp
+++ b/llvm/lib/Analysis/Lint.cpp
@@ -747,10 +747,13 @@ PreservedAnalyses LintPass::run(Function &F, FunctionAnalysisManager &AM) {
   Lint L(Mod, DL, AA, AC, DT, TLI);
   L.visit(F);
   dbgs() << L.MessagesStr.str();
-  if (LintAbortOnError && !L.MessagesStr.str().empty())
-    report_fatal_error(Twine("Linter found errors, aborting. (enabled by --") +
+  if (!L.MessagesStr.str().empty()) {
+    F.getParent()->IsValid = false;
+    if (LintAbortOnError)
+      report_fatal_error(Twine("Linter found errors, aborting. (enabled by --") +
                            LintAbortOnErrorArgName + ")",
-                       false);
+                         false);
+  }
   return PreservedAnalyses::all();
 }
 
diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index 7b6f7b5aa6171..26616c4de59d2 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -135,6 +135,10 @@ static cl::opt<bool> VerifyNoAliasScopeDomination(
     cl::desc("Ensure that llvm.experimental.noalias.scope.decl for identical "
              "scopes are not dominating"));
 
+static cl::opt<bool>
+    VerifyAbortOnError("verifier-abort-on-error", cl::init(false),
+                       cl::desc("In the Verifier pass, abort on errors."));
+
 namespace llvm {
 
 struct VerifierSupport {
@@ -7739,16 +7743,22 @@ VerifierAnalysis::Result VerifierAnalysis::run(Function &F,
 
 PreservedAnalyses VerifierPass::run(Module &M, ModuleAnalysisManager &AM) {
   auto Res = AM.getResult<VerifierAnalysis>(M);
-  if (FatalErrors && (Res.IRBroken || Res.DebugInfoBroken))
-    report_fatal_error("Broken module found, compilation aborted!");
+  if (Res.IRBroken || Res.DebugInfoBroken) {
+    M.IsValid = false;
+    if (VerifyAbortOnError && FatalErrors)
+      report_fatal_error("Broken module found, compilation aborted!");
+  }
 
   return PreservedAnalyses::all();
 }
 
 PreservedAnalyses VerifierPass::run(Function &F, FunctionAnalysisManager &AM) {
   auto res = AM.getResult<VerifierAnalysis>(F);
-  if (res.IRBroken && FatalErrors)
-    report_fatal_error("Broken function found, compilation aborted!");
+  if (res.IRBroken) {
+    F.getParent()->IsValid = false;
+    if (VerifyAbortOnError && FatalErrors)
+      report_fatal_error("Broken function found, compilation aborted!");
+  }
 
   return PreservedAnalyses::all();
 }
diff --git a/llvm/lib/LTO/LTOBackend.cpp b/llvm/lib/LTO/LTOBackend.cpp
index 8a2dddce4892c..2d2f16005bd40 100644
--- a/llvm/lib/LTO/LTOBackend.cpp
+++ b/llvm/lib/LTO/LTOBackend.cpp
@@ -270,7 +270,7 @@ static void runNewPMPasses(const Config &Conf, Module &Mod, TargetMachine *TM,
   PassInstrumentationCallbacks PIC;
   StandardInstrumentations SI(Mod.getContext(), Conf.DebugPassManager,
                               Conf.VerifyEach);
-  SI.registerCallbacks(PIC, &MAM);
+  SI.registerCallbacks(PIC, &MAM, &FAM);
   PassBuilder PB(TM, Conf.PTO, PGOOpt, &PIC);
 
   RegisterPassPlugins(Conf.PassPlugins, PB);
diff --git a/llvm/lib/LTO/ThinLTOCodeGenerator.cpp b/llvm/lib/LTO/ThinLTOCodeGenerator.cpp
index 189f2876b61c0..84df56948a5a7 100644
--- a/llvm/lib/LTO/ThinLTOCodeGenerator.cpp
+++ b/llvm/lib/LTO/ThinLTOCodeGenerator.cpp
@@ -245,7 +245,7 @@ static void optimizeModule(Module &TheModule, TargetMachine &TM,
 
   PassInstrumentationCallbacks PIC;
   StandardInstrumentations SI(TheModule.getContext(), DebugPassManager);
-  SI.registerCallbacks(PIC, &MAM);
+  SI.registerCallbacks(PIC, &MAM, &FAM);
   PipelineTuningOptions PTO;
   PTO.LoopVectorization = true;
   PTO.SLPVectorization = true;
diff --git a/llvm/lib/Passes/CMakeLists.txt b/llvm/lib/Passes/CMakeLists.txt
index 23799ac4f98f7..4b643837722ad 100644
--- a/llvm/lib/Passes/CMakeLists.txt
+++ b/llvm/lib/Passes/CMakeLists.txt
@@ -30,6 +30,7 @@ add_llvm_component_library(LLVMPasses
   Scalar
   Support
   Target
+  TargetParser
   TransformUtils
   Vectorize
   Instrumentation
diff --git a/llvm/lib/Passes/PassBuilderBindings.cpp b/llvm/lib/Passes/PassBuilderBindings.cpp
index 933fe89e53a94..f0e1abb8cebc4 100644
--- a/llvm/lib/Passes/PassBuilderBindings.cpp
+++ b/llvm/lib/Passes/PassBuilderBindings.cpp
@@ -76,7 +76,7 @@ static LLVMErrorRef runPasses(Module *Mod, Function *Fun, const char *Passes,
   PB.crossRegisterProxies(LAM, FAM, CGAM, MAM);
 
   StandardInstrumentations SI(Mod->getContext(), Debug, VerifyEach);
-  SI.registerCallbacks(PIC, &MAM);
+  SI.registerCallbacks(PIC, &MAM, &FAM);
 
   // Run the pipeline.
   if (Fun) {
diff --git a/llvm/lib/Passes/StandardInstrumentations.cpp b/llvm/lib/Passes/StandardInstrumentations.cpp
index b766517e68eba..27ea19302d8d8 100644
--- a/llvm/lib/Passes/StandardInstrumentations.cpp
+++ b/llvm/lib/Passes/StandardInstrumentations.cpp
@@ -45,6 +45,7 @@
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Support/xxhash.h"
+#include "llvm/Target/TargetVerifier.h"
 #include <unordered_map>
 #include <unordered_set>
 #include <utility>
@@ -1461,9 +1462,10 @@ void PreservedCFGCheckerInstrumentation::registerCallbacks(
 }
 
 void VerifyInstrumentation::registerCallbacks(PassInstrumentationCallbacks &PIC,
-                                              ModuleAnalysisManager *MAM) {
+                                              ModuleAnalysisManager *MAM,
+					      FunctionAnalysisManager *FAM) {
   PIC.registerAfterPassCallback(
-      [this, MAM](StringRef P, Any IR, const PreservedAnalyses &PassPA) {
+      [this, MAM, FAM](StringRef P, Any IR, const PreservedAnalyses &PassPA) {
         if (isIgnored(P) || P == "VerifierPass")
           return;
         const auto *F = unwrapIR<Function>(IR);
@@ -1480,6 +1482,15 @@ void VerifyInstrumentation::registerCallbacks(PassInstrumentationCallbacks &PIC,
             report_fatal_error(formatv("Broken function found after pass "
                                        "\"{0}\", compilation aborted!",
                                        P));
+
+          if (FAM) {
+            TargetVerify TV(const_cast<Module*>(F->getParent()));
+            TV.run(*const_cast<Function*>(F), *FAM);
+	    if (!F->getParent()->IsValid)
+              report_fatal_error(formatv("Broken function found after pass "
+                                         "\"{0}\", compilation aborted!",
+                                         P));
+          }
         } else {
           const auto *M = unwrapIR<Module>(IR);
           if (!M) {
@@ -2524,7 +2535,7 @@ void PrintCrashIRInstrumentation::registerCallbacks(
 }
 
 void StandardInstrumentations::registerCallbacks(
-    PassInstrumentationCallbacks &PIC, ModuleAnalysisManager *MAM) {
+    PassInstrumentationCallbacks &PIC, ModuleAnalysisManager *MAM, FunctionAnalysisManager *FAM) {
   PrintIR.registerCallbacks(PIC);
   PrintPass.registerCallbacks(PIC);
   TimePasses.registerCallbacks(PIC);
@@ -2533,7 +2544,7 @@ void StandardInstrumentations::registerCallbacks(
   PrintChangedIR.registerCallbacks(PIC);
   PseudoProbeVerification.registerCallbacks(PIC);
   if (VerifyEach)
-    Verify.registerCallbacks(PIC, MAM);
+    Verify.registerCallbacks(PIC, MAM, FAM);
   PrintChangedDiff.registerCallbacks(PIC);
   WebsiteChangeReporter.registerCallbacks(PIC);
   ChangeTester.registerCallbacks(PIC);
diff --git a/llvm/lib/Target/AMDGPU/AMDGPUTargetVerifier.cpp b/llvm/lib/Target/AMDGPU/AMDGPUTargetVerifier.cpp
new file mode 100644
index 0000000000000..e6cdec7160229
--- /dev/null
+++ b/llvm/lib/Target/AMDGPU/AMDGPUTargetVerifier.cpp
@@ -0,0 +1,213 @@
+#include "llvm/Target/TargetVerify/AMDGPUTargetVerifier.h"
+
+#include "llvm/Analysis/UniformityAnalysis.h"
+#include "llvm/Analysis/PostDominators.h"
+#include "llvm/Support/Debug.h"
+#include "llvm/IR/Dominators.h"
+#include "llvm/IR/Function.h"
+#include "llvm/IR/IntrinsicInst.h"
+#include "llvm/IR/IntrinsicsAMDGPU.h"
+#include "llvm/IR/Module.h"
+#include "llvm/IR/Value.h"
+
+#include "llvm/Support/raw_ostream.h"
+
+using namespace llvm;
+
+//static cl::opt<bool>
+//MarkUniform("mark-uniform", cl::desc("Mark instructions as uniform"), cl::init(false));
+
+// Check - We know that cond should be true, if not print an error message.
+#define Check(C, ...)                                                          \
+  do {                                                                         \
+    if (!(C)) {                                                                \
+      TargetVerify::CheckFailed(__VA_ARGS__);                                  \
+      return;                                                                  \
+    }                                                                          \
+  } while (false)
+
+static bool isMFMA(unsigned IID) {
+  switch (IID) {
+    case Intrinsic::amdgcn_mfma_f32_4x4x1f32:
+    case Intrinsic::amdgcn_mfma_f32_4x4x4f16:
+    case Intrinsic::amdgcn_mfma_i32_4x4x4i8:
+    case Intrinsic::amdgcn_mfma_f32_4x4x2bf16:
+
+    case Intrinsic::amdgcn_mfma_f32_16x16x1f32:
+    case Intrinsic::amdgcn_mfma_f32_16x16x4f32:
+    case Intrinsic::amdgcn_mfma_f32_16x16x4f16:
+    case Intrinsic::amdgcn_mfma_f32_16x16x16f16:
+    case Intrinsic::amdgcn_mfma_i32_16x16x4i8:
+    case Intrinsic::amdgcn_mfma_i32_16x16x16i8:
+    case Intrinsic::amdgcn_mfma_f32_16x16x2bf16:
+    case Intrinsic::amdgcn_mfma_f32_16x16x8bf16:
+
+    case Intrinsic::amdgcn_mfma_f32_32x32x1f32:
+    case Intrinsic::amdgcn_mfma_f32_32x32x2f32:
+    case Intrinsic::amdgcn_mfma_f32_32x32x4f16:
+    case Intrinsic::amdgcn_mfma_f32_32x32x8f16:
+    case Intrinsic::amdgcn_mfma_i32_32x32x4i8:
+    case Intrinsic::amdgcn_mfma_i32_32x32x8i8:
+    case Intrinsic::amdgcn_mfma_f32_32x32x2bf16:
+    case Intrinsic::amdgcn_mfma_f32_32x32x4bf16:
+
+    case Intrinsic::amdgcn_mfma_f32_4x4x4bf16_1k:
+    case Intrinsic::amdgcn_mfma_f32_16x16x4bf16_1k:
+    case Intrinsic::amdgcn_mfma_f32_16x16x16bf16_1k:
+    case Intrinsic::amdgcn_mfma_f32_32x32x4bf16_1k:
+    case Intrinsic::amdgcn_mfma_f32_32x32x8bf16_1k:
+
+    case Intrinsic::amdgcn_mfma_f64_16x16x4f64:
+    case Intrinsic::amdgcn_mfma_f64_4x4x4f64:
+
+    case Intrinsic::amdgcn_mfma_i32_16x16x32_i8:
+    case Intrinsic::amdgcn_mfma_i32_32x32x16_i8:
+    case Intrinsic::amdgcn_mfma_f32_16x16x8_xf32:
+    case Intrinsic::amdgcn_mfma_f32_32x32x4_xf32:
+
+    case Intrinsic::amdgcn_mfma_f32_16x16x32_bf8_bf8:
+    case Intrinsic::amdgcn_mfma_f32_16x16x32_bf8_fp8:
+    case Intrinsic::amdgcn_mfma_f32_16x16x32_fp8_bf8:
+    case Intrinsic::amdgcn_mfma_f32_16x16x32_fp8_fp8:
+
+    case Intrinsic::amdgcn_mfma_f32_32x32x16_bf8_bf8:
+    case Intrinsic::amdgcn_mfma_f32_32x32x16_bf8_fp8:
+    case Intrinsic::amdgcn_mfma_f32_32x32x16_fp8_bf8:
+    case Intrinsic::amdgcn_mfma_f32_32x32x16_fp8_fp8:
+      return true;
+    default:
+      return false;
+  }
+}
+
+namespace llvm {
+/*class AMDGPUTargetVerify : public TargetVerify {
+public:
+  Module *Mod;
+
+  DominatorTree *DT;
+  PostDominatorTree *PDT;
+  UniformityInfo *UA;
+
+  AMDGPUTargetVerify(Module *Mod, DominatorTree *DT, PostDominatorTree *PDT, UniformityInfo *UA)
+    : TargetVerify(Mod), Mod(Mod), DT(DT), PDT(PDT), UA(UA) {}
+
+  void run(Function &F);
+};*/
+
+static bool IsValidInt(const Type *Ty) {
+  return Ty->isIntegerTy(1) ||
+         Ty->isIntegerTy(8) ||
+         Ty->isIntegerTy(16) ||
+         Ty->isIntegerTy(32) ||
+         Ty->isIntegerTy(64) ||
+         Ty->isIntegerTy(128);
+}
+
+static bool isShader(CallingConv::ID CC) {
+  switch(CC) {
+    case CallingConv::AMDGPU_VS:
+    case CallingConv::AMDGPU_LS:
+    case CallingConv::AMDGPU_HS:
+    case CallingConv::AMDGPU_ES:
+    case CallingConv::AMDGPU_GS:
+    case CallingConv::AMDGPU_PS:
+    case CallingConv::AMDGPU_CS_Chain:
+    case CallingConv::AMDGPU_CS_ChainPreserve:
+    case CallingConv::AMDGPU_CS:
+      return true;
+    default:
+      return false;
+  }
+}
+
+void AMDGPUTargetVerify::run(Function &F) {
+  // Ensure shader calling convention returns void
+  if (isShader(F.getCallingConv()))
+    Check(F.getReturnType() == Type::getVoidTy(F.getContext()), "Shaders must return void");
+
+  for (auto &BB : F) {
+
+    for (auto &I : BB) {
+      //if (MarkUniform)
+        //outs() << UA->isUniform(&I) << ' ' << I << '\n';
+
+      // Ensure integral types are valid: i8, i16, i32, i64, i128
+      if (I.getType()->isIntegerTy())
+        Check(IsValidInt(I.getType()), "Int type is invalid.", &I);
+      for (unsigned i = 0; i < I.getNumOperands(); ++i)
+        if (I.getOperand(i)->getType()->isIntegerTy())
+          Check(IsValidInt(I.getOperand(i)->getType()),
+                "Int type is invalid.", I.getOperand(i));
+
+      // Ensure no store to const memory
+      if (auto *SI = dyn_cast<StoreInst>(&I))
+      {
+        unsigned AS = SI->getPointerAddressSpace();
+        Check(AS != 4, "Write to const memory", SI);
+      }
+
+      // Ensure no kernel to kernel calls.
+      if (auto *CI = dyn_cast<CallInst>(&I))
+      {
+        CallingConv::ID CalleeCC = CI->getCallingConv();
+        if (CalleeCC == CallingConv::AMDGPU_KERNEL)
+        {
+          CallingConv::ID CallerCC = CI->getParent()->getParent()->getCallingConv();
+          Check(CallerCC != CallingConv::AMDGPU_KERNEL,
+            "A kernel may not call a kernel", CI->getParent()->getParent());
+        }
+      }
+
+      // Ensure MFMA is not in control flow with diverging operands
+      if (auto *II = dyn_cast<IntrinsicInst>(&I)) {
+        if (isMFMA(II->getIntrinsicID())) {
+          bool InControlFlow = false;
+          for (const auto &P : predecessors(&BB))
+            if (!PDT->dominates(&BB, P)) {
+              InControlFlow = true;
+              break;
+            }
+          for (const auto &S : successors(&BB))
+            if (!DT->dominates(&BB, S)) {
+ ...
[truncated]

Comment on lines 217 to 219
/// Is this Module valid as determined by one of the verification passes
/// i.e. Lint, Verifier, TargetVerifier.
bool IsValid = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be added

if (!L.MessagesStr.str().empty()) {
F.getParent()->IsValid = false;
if (LintAbortOnError)
report_fatal_error(Twine("Linter found errors, aborting. (enabled by --") +
Copy link
Contributor

Choose a reason for hiding this comment

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

Error messages should start with lowercase

F.getParent()->IsValid = false;
if (LintAbortOnError)
report_fatal_error(Twine("Linter found errors, aborting. (enabled by --") +
LintAbortOnErrorArgName + ")",
Copy link
Contributor

Choose a reason for hiding this comment

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

The command line option is gone, this is only a pass parameter now

if (AbortOnError && !L.MessagesStr.str().empty())
report_fatal_error(
"linter found errors, aborting. (enabled by abort-on-error)", false);
if (!L.MessagesStr.str().empty()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't need the str()?

report_fatal_error(
"linter found errors, aborting. (enabled by abort-on-error)", false);
if (!L.MessagesStr.str().empty()) {
F.getParent()->IsValid = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

The IR may not be modified

Comment on lines 150 to 160
// Ensure no kernel to kernel calls.
if (auto *CI = dyn_cast<CallInst>(&I))
{
CallingConv::ID CalleeCC = CI->getCallingConv();
if (CalleeCC == CallingConv::AMDGPU_KERNEL)
{
CallingConv::ID CallerCC = CI->getParent()->getParent()->getCallingConv();
Check(CallerCC != CallingConv::AMDGPU_KERNEL,
"A kernel may not call a kernel", CI->getParent()->getParent());
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be a proper IR verifier error, and in the generic one

}
}

// Ensure MFMA is not in control flow with diverging operands
Copy link
Contributor

Choose a reason for hiding this comment

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

This is more of a lint check. Also it's not going to be reliable because we don't know if the value is known dynamically always uniform


// Ensure MFMA is not in control flow with diverging operands
if (auto *II = dyn_cast<IntrinsicInst>(&I)) {
if (isMFMA(II->getIntrinsicID())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

isMFMA isn't the right condition, barrier would fall in the same bucket

Comment on lines 3 to 6
define amdgpu_cs i32 @nonvoid_shader() {
; CHECK: LLVM ERROR
ret i32 0
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the generic IR verifier already gets this one. Also error messages need to be checked

@@ -0,0 +1,6 @@
; RUN: not not llc -mtriple=amdgcn -mcpu=gfx900 -verify-machineinstrs -enable-new-pm -verify-each -o - < %s 2>&1 | FileCheck %s
Copy link
Contributor

Choose a reason for hiding this comment

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

not not, also don't need all of these flags

@shiltian
Copy link
Contributor

shiltian commented Apr 2, 2025

Main thing is it would be really nice for this to be a hook that the usual verifier calls into as opposed to a separate pass, as verify is already in the debugging flow for lots of people.

I agree with this. It could be just a new function in TTI.

Adding the hook... Please let me know what you think.

It still looks like we have a dedicated pass.

@shiltian
Copy link
Contributor

shiltian commented Apr 8, 2025

reverse ping

shiltian added a commit that referenced this pull request Apr 8, 2025
… verifier

For AMDGPU, calls to entry functions are invalid. Previously, due to certain
limitations, this restriction was not enforced by the IR verifier. These
limitations have now been resolved, enabling us to enforce this check.

Adding target-dependent checks directly into the IR verifier is not ideal.
However, a cleaner solution, such as a dedicated target-dependent IR verifier,
is underway (e.g., #123609). Once that
or similar code is merged, we can move this check accordingly.
shiltian added a commit that referenced this pull request Apr 9, 2025
… verifier

For AMDGPU, calls to entry functions are invalid. Previously, due to certain
limitations, this restriction was not enforced by the IR verifier. These
limitations have now been resolved, enabling us to enforce this check.

Adding target-dependent checks directly into the IR verifier is not ideal.
However, a cleaner solution, such as a dedicated target-dependent IR verifier,
is underway (e.g., #123609). Once that
or similar code is merged, we can move this check accordingly.
shiltian added a commit that referenced this pull request Apr 9, 2025
… verifier

For AMDGPU, calls to entry functions are invalid. Previously, due to certain
limitations, this restriction was not enforced by the IR verifier. These
limitations have now been resolved, enabling us to enforce this check.

Adding target-dependent checks directly into the IR verifier is not ideal.
However, a cleaner solution, such as a dedicated target-dependent IR verifier,
is underway (e.g., #123609). Once that
or similar code is merged, we can move this check accordingly.
shiltian added a commit that referenced this pull request Apr 10, 2025
… verifier

For AMDGPU, calls to entry functions are invalid. Previously, due to certain
limitations, this restriction was not enforced by the IR verifier. These
limitations have now been resolved, enabling us to enforce this check.

Adding target-dependent checks directly into the IR verifier is not ideal.
However, a cleaner solution, such as a dedicated target-dependent IR verifier,
is underway (e.g., #123609). Once that
or similar code is merged, we can move this check accordingly.
shiltian added a commit that referenced this pull request Apr 10, 2025
… verifier

For AMDGPU, calls to entry functions are invalid. Previously, due to certain
limitations, this restriction was not enforced by the IR verifier. These
limitations have now been resolved, enabling us to enforce this check.

Adding target-dependent checks directly into the IR verifier is not ideal.
However, a cleaner solution, such as a dedicated target-dependent IR verifier,
is underway (e.g., #123609). Once that
or similar code is merged, we can move this check accordingly.
shiltian added a commit that referenced this pull request Apr 11, 2025
… verifier

For AMDGPU, calls to entry functions are invalid. Previously, due to certain
limitations, this restriction was not enforced by the IR verifier. These
limitations have now been resolved, enabling us to enforce this check.

Adding target-dependent checks directly into the IR verifier is not ideal.
However, a cleaner solution, such as a dedicated target-dependent IR verifier,
is underway (e.g., #123609). Once that
or similar code is merged, we can move this check accordingly.
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:codegen IR generation bugs: mangling, exceptions, etc. labels Apr 16, 2025
jofrn added 5 commits April 19, 2025 20:36
This pass verifies the IR for an individual backend. This is
different than Lint because it consolidates all checks for a
given backend in a single pass. A check for Lint may be
undefined behavior across all targets, whereas a check in
TargetVerifier would only pertain to the specified target
but can check more than just undefined behavior such are IR
validity. A use case of this would be to reject programs
with invalid IR while fuzzing.
@jofrn jofrn force-pushed the tgt-verifier branch 2 times, most recently from 6074b9f to b107422 Compare April 26, 2025 15:06
auto *DT = &AM.getResult<DominatorTreeAnalysis>(F);
auto *PDT = &AM.getResult<PostDominatorTreeAnalysis>(F);

AMDGPUTargetVerify TV(Mod, DT, PDT, UA);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused by how this works. If we are going to use polymorphism , I'd imagine we have a TargetVerifierPass as a base/interface class, and then each target creates its own instance. We will create the target specific one wherever we need.

On the other hand, if we are going to use call back, there is no need for a dedicated interface class for the verifier. We only need to make a target specific verifier as a module and/or function class.

However, what we have here is, a generic pass (at least its name looks like so) includes target specific header, and creates instance inside it.

Copy link
Contributor Author

@jofrn jofrn Apr 27, 2025

Choose a reason for hiding this comment

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

That is correct. They call into the target-specific implementation, and they check the triple. I can try this other way.

Copy link
Contributor Author

@jofrn jofrn Apr 28, 2025

Choose a reason for hiding this comment

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

We need the implementation of run defined in the base class even if the base is virtual because we want to instantiate a TargetVerify in llc's legacy PM, which should be able to use createTargetVerifierLegacyPass.

bool doInitialization(Module &M) override {
TV = std::make_unique<TargetVerify>(&M);
return false;
}

In other words, I believe we still need to check the Triple.

Copy link
Contributor

Choose a reason for hiding this comment

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

I still can't figure it out why we need TargetVerifier.h and TargetVerifier.cpp in the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are for instrumentation to run the pass in between every other one.

/// This is an interface that can be used to populate a
/// \c ModuleAnalysisManager with all registered loop analyses. Callers can
/// still manually register any additional analyses.
void registerVerifierPasses(ModulePassManager &PM, FunctionPassManager &);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why you need both module and function pass manager here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Simply because one is a FunctionPass and the other is a ModulePass.

Comment on lines 860 to 864
// Verifier callbacks
SmallVector<std::function<bool(ModulePassManager &)>, 2>
VerifierCallbacks;
SmallVector<std::function<bool(FunctionPassManager &)>, 2>
FnVerifierCallbacks;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary to have two here? We can use adaptor right?

@@ -0,0 +1,84 @@
//===-- llvm/Target/TargetVerifier.h - LLVM IR Target Verifier ---*- C++ -*-===//
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the value of this interface class instead of just making those target verifier a module/function pass?

Copy link
Contributor Author

@jofrn jofrn Apr 30, 2025

Choose a reason for hiding this comment

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

It is to just have a common interface to derive from, but we will not instantiate the base class as per the current method (since we are no longer putting it in instrumentation).

auto *DT = &AM.getResult<DominatorTreeAnalysis>(F);
auto *PDT = &AM.getResult<PostDominatorTreeAnalysis>(F);

AMDGPUTargetVerify TV(Mod, DT, PDT, UA);
Copy link
Contributor

Choose a reason for hiding this comment

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

I still can't figure it out why we need TargetVerifier.h and TargetVerifier.cpp in the first place.

@@ -0,0 +1,34 @@
set(LLVM_LINK_COMPONENTS
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know why we want a dedicated tool for this

@jofrn
Copy link
Contributor Author

jofrn commented Apr 30, 2025

I still can't figure it out why we need TargetVerifier.h and TargetVerifier.cpp in the first place.

If we want to call TargetVerify from StandardInstrumentations.cpp, then we need it because this is a target-independent location to run TargetVerify in between each pass. Working on removing it at the moment.

@jofrn jofrn force-pushed the tgt-verifier branch 5 times, most recently from aae6d80 to c3cb351 Compare April 30, 2025 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:AMDGPU clang:codegen IR generation bugs: mangling, exceptions, etc. clang Clang issues not falling into any other category llvm:analysis llvm:ir LTO Link time optimization (regular/full LTO or ThinLTO)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants