[PATCH] D144354: [WebAssembly] Split WebAssemblyUtils to fix library layering for MC tools.

Thu Feb 23 15:16:45 PST 2023

aheejin added inline comments.

================
Comment at: llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCUtilities.h:1
+//===-- WebAssemblyMCUtilities - WebAssembly Utility Functions -*- C++ -*-====//
+//
----------------
craig.topper wrote:
> aheejin wrote:
> > It looks `MCTargetDesc/WebAssemblyMCUtilities.h` and `MCTargetDesc/WebAssemblyMCUtilities.cpp` don't have any MC-related functionalities. Why do they need to be moved into `MCTargetDesc/`?
> Two of the options are used here.
> 
> ```
> WebAssemblyMCAsmInfo.cpp
> 53:  if (WebAssembly::WasmEnableEH || WebAssembly::WasmEnableSjLj)
> ```
> 
> I didn't look too closely to notice the other two aren't used in MC.
Ah, right... That was added in D115893, and the reason we needed that in `MCAsmInfo` was we couldn't determine the EH mode only with the triple. Yeah, this is not very ideal, but I can't think of any other way around this atm. Sorry for making you revert the change, but can we bring back the two other options (`WasmEnableEmEH` and `WasmEnableEmSjLj`) here? I think it's clearer to gather all EH/SjLj options in one place. Also given that this file only contains options, can we change the file name to, say, `WebAssemblyEHSjLjOptions.cpp/h`?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144354/new/

https://reviews.llvm.org/D144354