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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm 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);
----------------
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


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyMachineFunctionInfo.cpp:161
+
+  // FIXME: This link should be removed
+  if (WasmEHFuncInfo *WasmEHInfo = MF.getWasmEHFuncInfo()) {
----------------
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 


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyMachineFunctionInfo.h:34
 class WebAssemblyFunctionInfo final : public MachineFunctionInfo {
-  const MachineFunction *MF;
-
----------------
pmatos wrote:
> Back in https://github.com/llvm/llvm-project/commit/cc5a1b3dd9039d50f6b9caa679d60398f0cec65f you changed this from a reference to a pointer. Apparently this is not needed at all and you are removing it. I am happy with it. Looking through the code I get the feeling that this was an artifact from doing similar things as other backends. Some of which have ended up removing MF, while others still have it.
I purged this from all targets last time I posted this patch; this one was added back in at some point since then 


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

https://reviews.llvm.org/D137295