[llvm] [NVPTX] Skip processing BasicBlocks with single unreachable instruction in `nvptx-lower-unreachable` pass. (PR #72641)

via llvm-commits llvm-commits at lists.llvm.org
Thu Feb 15 13:45:20 PST 2024


================
@@ -138,7 +138,19 @@ bool NVPTXLowerUnreachable::runOnFunction(Function &F) {
   InlineAsm *Exit = InlineAsm::get(ExitFTy, "exit;", "", true);
 
   bool Changed = false;
-  for (auto &BB : F)
+
+  // In scenarios where a BasicBlock contains only one unreachable instruction,
+  // the joint action of nvptx-isel and unreachable-mbb-elimination
+  // effectively optimizes the BasicBlock out. However, adding an exit
+  // command to such a BasicBlock, as suggested by this pass, preserves it
+  // within the Control Flow Graph (CFG), thereby negatively impacting size and
+  // performance. To counteract this undesirable consequence, we choose to
+  // refrain from processing BasicBlocks with just one unreachable instruction
+  // in this pass.
+
----------------
mmoadeli wrote:

> You're still proposing to trade off correctness for performance and I still strongly believe it's not something we want to do.

I disagree with you on this. The `exit` with an additional node in CGF is introduced by  [this](https://reviews.llvm.org/D152789). If you build the test before that PR and with my change, you'll see they have identical ptx, whereas after that PR you'll see an additional node with `exit` in it.

If as claimed, this patch sacrifices correctness for performance, then before  [this](https://reviews.llvm.org/D152789) we did not have correct behaviour in this situations.

I'd suggest comparing the ptx of three revision. 1) before  [this](https://reviews.llvm.org/D152789), 2) after that, 3 after the fix introduced in this patch.



https://github.com/llvm/llvm-project/pull/72641


More information about the llvm-commits mailing list