[PATCH] D137295: WebAssembly: Remove MachineFunction reference from MFI

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyMachineFunctionInfo.cpp:31
     const {
-  WebAssemblyFunctionInfo *Clone =
-      DestMF.cloneInfo<WebAssemblyFunctionInfo>(*this);
-  Clone->MF = &DestMF;
-  return Clone;
+  // FIXME: This is broken if WasmEHFuncInfo from the block references.
+  return DestMF.cloneInfo<WebAssemblyFunctionInfo>(*this);
----------------
arsenm wrote:
> aheejin wrote:
> > arsenm wrote:
> > > aheejin wrote:
> > > > What does this mean by "if WasmEHFuncInfo from the block reference"?
> > > Ultimately WasmEHFuncInfo has a bunch of MachineBasicBlock pointers in it. When cloned to a new machine function, those need to be updated to point to the blocks in the new function which isn't handled
> > Apparently I'm not familiar with the cloning functionality you added a while ago. What is that for? Is that creating an in-memory copy of a while function, including BBs and instructions? Then what's src BB and what's dst BB? 
> > 
> > And how does `WinEHFuncInfo` handle it currently?
> It's for llvm-reduce; it's just unhandled now. If you try to reduce a function with EH info it won't faithfully reproduce 
- Then what are src and dst BBs? If you are cloning a given parameter, how do you know what the dst BB will be? Are BBs cloned first and supplied here?
- How about changing this to `TODO`? `TODO: Implement cloning for WasmEHFuncInfo` or something.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyMachineFunctionInfo.cpp:161
+
+  // FIXME: This link should be removed
+  if (WasmEHFuncInfo *WasmEHInfo = MF.getWasmEHFuncInfo()) {
----------------
arsenm wrote:
> aheejin wrote:
> > arsenm wrote:
> > > aheejin wrote:
> > > > Why? Which link?
> > > This is the layering violation where this field is stored in the MachineFunction, but all the handling is done here in target code. Either everything should be done here, or done in the generic MIR handling 
> > As I said, `WinEHFuncInfo` is also within `MachineFunction`. Also this `WasmEHFuncInfo` is not only for MIR handling. It is referenced throughout our compilation passes.
> Right, it's a well spread out layering violation. The handling should be consistently in webassembly only, or consistently in the generic MachineFunction handling
I think the reason both of them are in `MachineFunction` is, as noted above, they are for exception handling conventions, not for specific targets. (Well, `WinEHFuncInfo` kind of happens to be only used in x86, and `WasmEHFuncInfo` only in WebAssembly... but anyway)

As we don't have a plan atm to restructure this, how about removing this `FIXME`, or elaborating more on what the suggestion could be? "This link should be removed" doesn't explain much on why for future readers.


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

https://reviews.llvm.org/D137295