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

[Heejin Ahn via llvm-commits]

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

`DemoteCatchSwitchPHIOnly` option in `WinEHPrepare` pass was added in https://github.com/llvm/llvm-project/commit/99d60e0dabcf20f4db683da83cde905b7a1373de, because Wasm EH uses `WinEHPrepare`, but it didn'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

>From 60290eaf2f6383c8bfbb89f7297d3d1773773322 Mon Sep 17 00:00:00 2001
From: Heejin Ahn <aheejin at gmail.com>
Date: Tue, 13 Feb 2024 04:28:51 +0000
Subject: [PATCH] [WebAssembly] Demote PHIs in catchswitch BB only

`DemoteCatchSwitchPHIOnly` option in `WinEHPrepare` pass was added in
https://github.com/llvm/llvm-project/commit/99d60e0dabcf20f4db683da83cde905b7a1373de,
because Wasm EH uses `WinEHPrepare`, but it didn'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
---
 llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp | 3 ++-
 llvm/lib/CodeGen/TargetPassConfig.cpp                  | 2 +-
 llvm/test/CodeGen/WebAssembly/wasm-eh-prepare.ll       | 2 +-
 3 files changed, 4 insertions(+), 3 deletions(-)

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