[WebAssembly] Problems of forcing mem2reg in reference-types

[aheejin]

I recently noticed that we force mem2reg optimization to run unconditionally, even in -O0, when reference-types feature is enabled:

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.

I think this can have some problems:

1. This means we run some optimizations even when we are not supposed to. 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 even in -O0. Also, as it happens, we've been even discussing enabling reference-types by default. ([WebAssembly] Enable multivalue and reference-types in generic CPU config #80923) It'd be good if there's a way to promote only reference type values to registers.

2. When Wasm EH is used, we demote certain PHIs from registers to memory in WinEHPrepare. (Wasm EH uses WinEHPrepare pass, because it uses WinEH IR) Specifically, we cannot have a PHI before a catchswitch. 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 when reference-types is enabled, we force to run mem2reg in addISelPrepare, which runs after WinEHPrepare, re-promoting the demoted PHIs in WinEHPrepare, which crashes the compiler. This is a problem even if we have a way to promote only reference types because they can be before a catchswitch too.

Any thoughts or ideas?
cc @pmatos

[llvmbot]

@llvm/issue-subscribers-backend-webassembly

Author: Heejin Ahn (aheejin)

I recently noticed that we force mem2reg optimization to run unconditionally, even in `-O0`, when reference-types feature is enabled: 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.

I think this can have some problems:

1. This means we run some optimizations even when we are not supposed to. 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 even in -O0. Also, as it happens, we've been even discussing enabling reference-types by default. (#80923) It'd be good if there's a way to promote only reference type values to registers.

2. When Wasm EH is used, we demote certain PHIs from registers to memory in WinEHPrepare. (Wasm EH uses WinEHPrepare pass, because it uses WinEH IR) Specifically, we cannot have a PHI before a catchswitch. 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 when reference-types is enabled, we force to run mem2reg in addISelPrepare, which runs after WinEHPrepare, re-promoting the demoted PHIs in WinEHPrepare, which crashes the compiler. This is a problem even if we have a way to promote only reference types because they can be before a catchswitch too.

Any thoughts or ideas?
cc @pmatos

[pmatos]

@aheejin As it's in the comment we need to promote allocas for reftypes to registers and at the time mem2reg did that for us and it worked fine.

It seems that this causes issues with EH as you say. I think we might need a pass here dedicated to just promoting reftypes to allocas that's slightly more constrained - sort of a specialized mem2reg for reftypes.

[xortoast]

@pmatos Do we need to promote allocas to registers? I think promoting them to locals should be enough, which can be done by moving them from WASM_ADDRESS_SPACE_DEFAULT to WASM_ADDRESS_SPACE_VAR. This would also solve the issues with EH.

[pmatos]

@pmatos Do we need to promote allocas to registers? I think promoting them to locals should be enough, which can be done by moving them from WASM_ADDRESS_SPACE_DEFAULT to WASM_ADDRESS_SPACE_VAR. This would also solve the issues with EH.

It has been over a year since I looked into this but if memory serves me right, I don't think we have a way to do that in IR atm.

[xortoast]

I don't think we have a way to do that in IR atm.

The alloca instruction accepts an addrspace attribute, and addrspace(1) can be used to lower them to local variables (added in 82f92e3).

The only thing needed would be a new mem2local pass that adds the addrspace(1) to promotable allocas.

[aheejin]

@xortoast Thanks for the info! Currently it looks Wasm reference types are represented by ptrs to addrspace(10) (for externref) or addrspace(20) (for funcref). If they become ptrs to addrspace(1), their original addrspace info will be lost. Then do you have any suggestions on how we can encode values' type info (externref or funcref) after they are moved to addrspace(1)?

[xortoast]

@aheejin I think you misunderstood a bit. Reference types are represented as ptr addrspace(10) or ptr addrspace(20), but the alloca itself lives in another address space.

For example, a local variable of type externref currently compiles to %1 = alloca ptr addrspace(10), align 1, where %1 has type ptr.

After running mem2local, the same alloca becomes %1 = alloca ptr addrspace(10), align 1, addrspace(1), where %1 has type ptr addrspace(1).

[aheejin]

@xortoast Thanks! Added that pass in #81965.