[llvm] [WebAssembly] Demote PHIs in catchswitch BB only (PR #81570)

[via llvm-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-llvm-selectiondag

@llvm/pr-subscribers-backend-webassembly

Author: Heejin Ahn (aheejin)

<details>
<summary>Changes</summary>

`DemoteCatchSwitchPHIOnly` option in `WinEHPrepare` pass was added in https://github.com/llvm/llvm-project/commit/99d60e0dabcf20f4db683da83cde905b7a1373de, because Wasm EH uses `WinEHPrepare`, but it doesn't need to demote all PHIs. PHIs in `catchswitch` BBs have to be removed (= demoted) because `catchswitch`s are removed in ISel and `catchswitch` BBs are removed as well, so they can't have other instructions.

But because Wasm EH doesn't use funclets, so PHIs in `catchpad` or `cleanuppad` BBs don't need to be demoted. That was the reason `DemoteCatchSwitchPHIOnly` option was added, in order not to demote more instructions unnecessarily.

The problem is it should have been set to `true` for Wasm EH. (Its default value is `false` for WinEH) And I mistakenly set it to `false` and wasn't aware about this for more than 5 years. This was not the end of the world; it just means we've been demoting more instructions than we should, possibly huting code size. In practice I think had hardly any effect in real performance given that the occurrence of PHIs in `catchpad` or `cleanuppad` BBs are not very frequent and many people run other optimizers like Binaryen anyway.

---

Not directly related to this PR but on a related note,

This was discovered by chance after enabling reference-types in Clang triggered this error:
https://github.com/llvm/llvm-project/blob/2dd52046816ac706a25d588efac77705793d8778/llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp#L252-L253 which was strange because this should've been triggered many times already, and I realized we had been setting `DemoteCatchSwitchPHIOnly` to false all along.

The reason it was triggered by 'reference-types' feature was apparently we run `mem2reg` unconditionally when 'reference-types' is enabled, which runs in `addISelPrepare` and happened to re-promote demoted PHIs by `WinEHPrepare`:
https://github.com/llvm/llvm-project/blob/2dd52046816ac706a25d588efac77705793d8778/llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp#L479-L482 (This was added in
 https://github.com/llvm/llvm-project/commit/890146b19206827bc48ee1ae1dc1534ff2ff18d7)

It looks this was in order to promote reference type values to registers because they cannot be stored in memory. But this has an effect of promoting values of all types, effectively running optimization even in `-O0`, which I'm not sure is OK. For example this means we will handle debug value differently and debug info can be lost even in `-O0`, if reference-types is enabled. And the newest EH spec requires reference-types to be enabled, so this means enabling EH means running some optimizations on `-O0`. It'd be good if there's a way to promote only reference type values to registers... cc @<!-- -->pmatos

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


3 Files Affected:

- (modified) llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp (+2-1) 
- (modified) llvm/lib/CodeGen/TargetPassConfig.cpp (+1-1) 
- (modified) llvm/test/CodeGen/WebAssembly/wasm-eh-prepare.ll (+1-1) 


``````````diff
diff --git a/llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp b/llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp
index 6cf54085915230..4172fbc96d1e5d 100644
--- a/llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp
@@ -249,7 +249,8 @@ void FunctionLoweringInfo::set(const Function &fn, MachineFunction &mf,
                "WinEHPrepare failed to remove PHIs from imaginary BBs");
         continue;
       }
-      if (isa<FuncletPadInst>(PadInst))
+      if (isa<FuncletPadInst>(PadInst) &&
+          Personality != EHPersonality::Wasm_CXX)
         assert(&*BB.begin() == PadInst && "WinEHPrepare failed to demote PHIs");
     }
 
diff --git a/llvm/lib/CodeGen/TargetPassConfig.cpp b/llvm/lib/CodeGen/TargetPassConfig.cpp
index e82f14e878141a..2ed39a5696e205 100644
--- a/llvm/lib/CodeGen/TargetPassConfig.cpp
+++ b/llvm/lib/CodeGen/TargetPassConfig.cpp
@@ -918,7 +918,7 @@ void TargetPassConfig::addPassesToHandleExceptions() {
     // on catchpads and cleanuppads because it does not outline them into
     // funclets. Catchswitch blocks are not lowered in SelectionDAG, so we
     // should remove PHIs there.
-    addPass(createWinEHPass(/*DemoteCatchSwitchPHIOnly=*/false));
+    addPass(createWinEHPass(/*DemoteCatchSwitchPHIOnly=*/true));
     addPass(createWasmEHPass());
     break;
   case ExceptionHandling::None:
diff --git a/llvm/test/CodeGen/WebAssembly/wasm-eh-prepare.ll b/llvm/test/CodeGen/WebAssembly/wasm-eh-prepare.ll
index bd577e387c72b7..164c138cb7578e 100644
--- a/llvm/test/CodeGen/WebAssembly/wasm-eh-prepare.ll
+++ b/llvm/test/CodeGen/WebAssembly/wasm-eh-prepare.ll
@@ -2,6 +2,7 @@
 ; RUN: opt < %s -win-eh-prepare -demote-catchswitch-only -wasm-eh-prepare -S --mattr=+atomics,+bulk-memory | FileCheck %s
 ; RUN: opt < %s -passes='win-eh-prepare<demote-catchswitch-only>,wasm-eh-prepare' -S | FileCheck %s
 ; RUN: opt < %s -passes='win-eh-prepare<demote-catchswitch-only>,wasm-eh-prepare' -S --mattr=+atomics,+bulk-memory | FileCheck %s
+; RUN: llc < %s -wasm-enable-eh -exception-model=wasm -mattr=+exception-handling -stop-after=wasm-eh-prepare | FileCheck %s
 
 target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
 target triple = "wasm32-unknown-unknown"
@@ -245,7 +246,6 @@ bb.true:                                          ; preds = %entry
 bb.true.0:                                        ; preds = %bb.true
   br label %merge
 
-; CHECK:      bb.false
 bb.false:                                         ; preds = %entry
   br label %merge
 

``````````

</details>


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