[SPIR-V] Replace assert with report_fatal

[Keenuts]

Irreducible must always be rejected, not only in debug builds.

[llvmbot]

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

Author: Nathan Gauër (Keenuts)

Changes

Irreducible must always be rejected, not only in debug builds.

---

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

1 Files Affected:

• (modified) llvm/lib/Target/SPIRV/SPIRVUtils.cpp (+3-2)

diff --git a/llvm/lib/Target/SPIRV/SPIRVUtils.cpp b/llvm/lib/Target/SPIRV/SPIRVUtils.cpp
index 50338f5df90281..6f30638144d885 100644
--- a/llvm/lib/Target/SPIRV/SPIRVUtils.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVUtils.cpp
@@ -570,8 +570,9 @@ size_t PartialOrderingVisitor::visit(BasicBlock *BB, size_t Unused) {
 
     if (!CanBeVisited(BB)) {
       ToVisit.push(BB);
-      assert(QueueIndex < ToVisit.size() &&
-             "No valid candidate in the queue. Is the graph reducible?");
+      if (QueueIndex >= ToVisit.size())
+        llvm::report_fatal_error(
+            "No valid candidate in the queue. Is the graph reducible?");
       QueueIndex++;
       continue;
     }

[VyacheslavLevytskyy]

I'm not quite sure I understand the logics. We are looking into an optional step, that is an attempt to sort basic blocks. My expectation would be that it never fails, because it's an optional step that not of applications require or even ask for.

I think the correct way to deal with any case where this method doesn't work is to return without changing anything and pass info about the fail of sortBlocks() to the later stage, so that only those of applications which require this step are free to fail with diagnostics kind of "No valid candidate in the queue. Is the graph reducible?"

@s-perron What do you think about the case?

[Keenuts]

I think the correct way to deal with any case where this method doesn't work is to return without changing anything and pass info about the fail of sortBlocks()

Note that since #116996 the visitor is not used for block sorting anymore. It's use-case is only in the structurizer which has tighter requirements than sortBlocks

[s-perron]

I thought this was an analysis/visitor that is intended to visit the blocks in a certain order. It was used to order the blocks, but that had changed.

If we cannot traverse the block in the expected order, we should issue an error. Algorithms using it could silently do the wrong thing.

I prefer having a prerequisite the graph must be reducible, and enforcing it. Then it is the callers responsibility to explicitly decide what to do with irreducible graphs.

[Keenuts]

I thought this was an analysis/visitor that is intended to visit the blocks in a certain order. It was used to order the blocks, but that had changed.

👍 That's the case. sortBlocks don't use it anymore. The hang was caused by a death test I added which passed a irreducible to the visitor to make sure it asserted. But if asserts are disabled, this test hang.

[VyacheslavLevytskyy]

thank you for the explanation @Keenuts