[SPIR-V] Duplicates Tracker accounts for possible changes in Constant usage after optimization

[VyacheslavLevytskyy]

This PR introduces changes into processing of internal/service data in SPIRV Backend so that Duplicates Tracker accounts for possible changes in Constant usage after optimization, namely this PR fixes the case when a Constant register stored in Duplicates Tracker after all passes is represented by a non-constant expression. In this case we may be sure that it neither is able to create a duplicate nor is in need of a special placement as a Constant instruction.

This PR doesn't introduce a new feature, and in this case we rely on existing set of test cases in the SPIRV Backend test suite to ensure that this PR doesn't break existing assumptions without introducing new test cases. There is a reproducer of the issue available as part of SYCL CTS test suite, however it's a binary of several MB's size. Given the subtlety of the issue, reduction of the reproducer to a reasonable site for inclusion into the SPIRV Backend test suite doesn't seem realistic.

[llvmbot]

@llvm/pr-subscribers-backend-spir-v

Author: Vyacheslav Levytskyy (VyacheslavLevytskyy)

Changes

This PR introduces changes into processing of internal/service data in SPIRV Backend so that Duplicates Tracker accounts for possible changes in Constant usage after optimization, namely this PR fixes the case when a Constant register stored in Duplicates Tracker after all passes is represented by a non-constant expression. In this case we may be sure that it neither is able to create a duplicate nor is in need of a special placement as a Constant instruction. This PR doesn't introduce a new feature, and in this case we rely on existing set of test cases in the SPIRV Backend test suite to ensure that this PR doesn't break existing assumptions without introducing new test cases.

---

Full diff: https://github.com/llvm/llvm-project/pull/110835.diff

4 Files Affected:

• (modified) llvm/lib/Target/SPIRV/SPIRVDuplicatesTracker.cpp (+25-24)
• (modified) llvm/lib/Target/SPIRV/SPIRVDuplicatesTracker.h (+12-3)
• (modified) llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.h (+2-1)
• (modified) llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp (+1-1)

diff --git a/llvm/lib/Target/SPIRV/SPIRVDuplicatesTracker.cpp b/llvm/lib/Target/SPIRV/SPIRVDuplicatesTracker.cpp
index 832ca0ba5a82d7..b82c2538a81368 100644
--- a/llvm/lib/Target/SPIRV/SPIRVDuplicatesTracker.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVDuplicatesTracker.cpp
@@ -12,6 +12,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "SPIRVDuplicatesTracker.h"
+#include "SPIRVInstrInfo.h"
 
 #define DEBUG_TYPE "build-dep-graph"
 
@@ -19,13 +20,14 @@ using namespace llvm;
 
 template <typename T>
 void SPIRVGeneralDuplicatesTracker::prebuildReg2Entry(
-    SPIRVDuplicatesTracker<T> &DT, SPIRVReg2EntryTy &Reg2Entry) {
+    SPIRVDuplicatesTracker<T> &DT, SPIRVReg2EntryTy &Reg2Entry,
+    const SPIRVInstrInfo *TII) {
   for (auto &TPair : DT.getAllUses()) {
     for (auto &RegPair : TPair.second) {
       const MachineFunction *MF = RegPair.first;
       Register R = RegPair.second;
       MachineInstr *MI = MF->getRegInfo().getUniqueVRegDef(R);
-      if (!MI)
+      if (!MI || (TPair.second.getIsConst() && !TII->isConstantInstr(*MI)))
         continue;
       Reg2Entry[&MI->getOperand(0)] = &TPair.second;
     }
@@ -33,16 +35,16 @@ void SPIRVGeneralDuplicatesTracker::prebuildReg2Entry(
 }
 
 void SPIRVGeneralDuplicatesTracker::buildDepsGraph(
-    std::vector<SPIRV::DTSortableEntry *> &Graph,
+    std::vector<SPIRV::DTSortableEntry *> &Graph, const SPIRVInstrInfo *TII,
     MachineModuleInfo *MMI = nullptr) {
   SPIRVReg2EntryTy Reg2Entry;
-  prebuildReg2Entry(TT, Reg2Entry);
-  prebuildReg2Entry(CT, Reg2Entry);
-  prebuildReg2Entry(GT, Reg2Entry);
-  prebuildReg2Entry(FT, Reg2Entry);
-  prebuildReg2Entry(AT, Reg2Entry);
-  prebuildReg2Entry(MT, Reg2Entry);
-  prebuildReg2Entry(ST, Reg2Entry);
+  prebuildReg2Entry(TT, Reg2Entry, TII);
+  prebuildReg2Entry(CT, Reg2Entry, TII);
+  prebuildReg2Entry(GT, Reg2Entry, TII);
+  prebuildReg2Entry(FT, Reg2Entry, TII);
+  prebuildReg2Entry(AT, Reg2Entry, TII);
+  prebuildReg2Entry(MT, Reg2Entry, TII);
+  prebuildReg2Entry(ST, Reg2Entry, TII);
 
   for (auto &Op2E : Reg2Entry) {
     SPIRV::DTSortableEntry *E = Op2E.second;
@@ -65,20 +67,19 @@ void SPIRVGeneralDuplicatesTracker::buildDepsGraph(
         if (MI->getOpcode() == SPIRV::OpConstantFunctionPointerINTEL && i == 2)
           continue;
         MachineOperand *RegOp = &VRegDef->getOperand(0);
-        LLVM_DEBUG({
-          if (Reg2Entry.count(RegOp) == 0 &&
-              (MI->getOpcode() != SPIRV::OpVariable || i != 3)) {
-            dbgs() << "Unexpected pattern while building a dependency "
-                      "graph.\nInstruction: ";
-            MI->print(dbgs());
-            dbgs() << "Operand: ";
-            Op.print(dbgs());
-            dbgs() << "\nOperand definition: ";
-            VRegDef->print(dbgs());
-          }
-        });
-        assert((MI->getOpcode() == SPIRV::OpVariable && i == 3) ||
-               Reg2Entry.count(RegOp));
+        if (Reg2Entry.count(RegOp) == 0 &&
+            (MI->getOpcode() != SPIRV::OpVariable || i != 3)) {
+          std::string DiagMsg;
+          raw_string_ostream OS(DiagMsg);
+          OS << "Unexpected pattern while building a dependency "
+                "graph.\nInstruction: ";
+          MI->print(OS);
+          OS << "Operand: ";
+          Op.print(OS);
+          OS << "\nOperand definition: ";
+          VRegDef->print(OS);
+          report_fatal_error(DiagMsg.c_str());
+        }
         if (Reg2Entry.count(RegOp))
           E->addDep(Reg2Entry[RegOp]);
       }
diff --git a/llvm/lib/Target/SPIRV/SPIRVDuplicatesTracker.h b/llvm/lib/Target/SPIRV/SPIRVDuplicatesTracker.h
index a37e65a47eda04..47391bf63b5947 100644
--- a/llvm/lib/Target/SPIRV/SPIRVDuplicatesTracker.h
+++ b/llvm/lib/Target/SPIRV/SPIRVDuplicatesTracker.h
@@ -26,6 +26,7 @@
 
 namespace llvm {
 namespace SPIRV {
+class SPIRVInstrInfo;
 // NOTE: using MapVector instead of DenseMap because it helps getting
 // everything ordered in a stable manner for a price of extra (NumKeys)*PtrSize
 // memory and expensive removals which do not happen anyway.
@@ -35,8 +36,9 @@ class DTSortableEntry : public MapVector<const MachineFunction *, Register> {
   struct FlagsTy {
     unsigned IsFunc : 1;
     unsigned IsGV : 1;
+    unsigned IsConst : 1;
     // NOTE: bit-field default init is a C++20 feature.
-    FlagsTy() : IsFunc(0), IsGV(0) {}
+    FlagsTy() : IsFunc(0), IsGV(0), IsConst(0) {}
   };
   FlagsTy Flags;
 
@@ -45,8 +47,10 @@ class DTSortableEntry : public MapVector<const MachineFunction *, Register> {
   // require hoisting of params as well.
   bool getIsFunc() const { return Flags.IsFunc; }
   bool getIsGV() const { return Flags.IsGV; }
+  bool getIsConst() const { return Flags.IsConst; }
   void setIsFunc(bool V) { Flags.IsFunc = V; }
   void setIsGV(bool V) { Flags.IsGV = V; }
+  void setIsConst(bool V) { Flags.IsConst = V; }
 
   const SmallVector<DTSortableEntry *, 2> &getDeps() const { return Deps; }
   void addDep(DTSortableEntry *E) { Deps.push_back(E); }
@@ -160,6 +164,10 @@ template <typename KeyTy> class SPIRVDuplicatesTrackerBase {
                      typename std::remove_const<
                          typename std::remove_pointer<KeyTy>::type>::type>())
       Storage[V].setIsGV(true);
+    if (std::is_same<Constant,
+                     typename std::remove_const<
+                         typename std::remove_pointer<KeyTy>::type>::type>())
+      Storage[V].setIsConst(true);
   }
 
   Register find(KeyTy V, const MachineFunction *MF) const {
@@ -211,11 +219,12 @@ class SPIRVGeneralDuplicatesTracker {
 
   template <typename T>
   void prebuildReg2Entry(SPIRVDuplicatesTracker<T> &DT,
-                         SPIRVReg2EntryTy &Reg2Entry);
+                         SPIRVReg2EntryTy &Reg2Entry,
+                         const SPIRVInstrInfo *TII);
 
 public:
   void buildDepsGraph(std::vector<SPIRV::DTSortableEntry *> &Graph,
-                      MachineModuleInfo *MMI);
+                      const SPIRVInstrInfo *TII, MachineModuleInfo *MMI);
 
   void add(const Type *Ty, const MachineFunction *MF, Register R) {
     TT.add(unifyPtrType(Ty), MF, R);
diff --git a/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.h b/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.h
index ace5cfe91ebe48..d301e119e16c8e 100644
--- a/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.h
+++ b/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.h
@@ -152,8 +152,9 @@ class SPIRVGlobalRegistry {
   }
 
   void buildDepsGraph(std::vector<SPIRV::DTSortableEntry *> &Graph,
+                      const SPIRVInstrInfo *TII,
                       MachineModuleInfo *MMI = nullptr) {
-    DT.buildDepsGraph(Graph, MMI);
+    DT.buildDepsGraph(Graph, TII, MMI);
   }
 
   void setBound(unsigned V) { Bound = V; }
diff --git a/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp b/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp
index 8908d8965b67c5..2083f18da9c590 100644
--- a/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp
@@ -282,7 +282,7 @@ void SPIRVModuleAnalysis::collectGlobalEntities(
 void SPIRVModuleAnalysis::processDefInstrs(const Module &M) {
   std::vector<SPIRV::DTSortableEntry *> DepsGraph;
 
-  GR->buildDepsGraph(DepsGraph, SPVDumpDeps ? MMI : nullptr);
+  GR->buildDepsGraph(DepsGraph, TII, SPVDumpDeps ? MMI : nullptr);
 
   collectGlobalEntities(
       DepsGraph, SPIRV::MB_TypeConstVars,