[WebAssembly] Demote PHIs in catchswitch BB only

[aheejin]

DemoteCatchSwitchPHIOnly option in WinEHPrepare pass was added in 99d60e0, because Wasm EH uses WinEHPrepare, but it doesn't need to demote all PHIs. PHIs in catchswitch BBs have to be removed (= demoted) because catchswitchs 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 it would've 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.

---

This was discovered by chance after enabling reference-types in Clang triggered this error:

llvm-project/llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp

Lines 252 to 253 in 2dd5204

	if (isa<FuncletPadInst>(PadInst))
	assert(&*BB.begin() == PadInst && "WinEHPrepare failed to demote PHIs");

which was strange because this should've been triggered many times already, and I realized we had been setting DemoteCatchSwitchPHIOnly to false all along.

But I think running mem2reg unconditionally when reference-types is enabled has its own set of problems... Will write about it in #81575.

[llvmbot]

@llvm/pr-subscribers-llvm-selectiondag

@llvm/pr-subscribers-backend-webassembly

Author: Heejin Ahn (aheejin)

Changes

DemoteCatchSwitchPHIOnly option in WinEHPrepare pass was added in 99d60e0, because Wasm EH uses WinEHPrepare, but it doesn't need to demote all PHIs. PHIs in catchswitch BBs have to be removed (= demoted) because catchswitchs 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:

llvm-project/llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp

Lines 252 to 253 in 2dd5204

	if (isa<FuncletPadInst>(PadInst))
	assert(&*BB.begin() == PadInst && "WinEHPrepare failed to demote PHIs");

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:

llvm-project/llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp

Lines 479 to 482 in 2dd5204

	if (Subtarget->hasReferenceTypes()) {
	// We need to remove allocas for reference types
	addPass(createPromoteMemoryToRegisterPass(true));
	}

(This was added in
890146b)

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 --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
 

[aheejin]

So far this was missed because we've been only running opt tests, with which we run -win-eh-prepare with -demote-catchswitch-only option manually. This adds a llc test that uses TargetPassConfig.cpp.

[aheejin]

This was deleted because, in llc tests many passes run before WinEHPrepare and WasmEHPrepare, and some of them did some simplification of the CFG, removing this BB.

[dschuff]

Very interesting finding!
I agree, I don't think we want to be running mem2reg all the time in O0