WebAssembly: Remove MachineFunction reference from MFI

[arsenm]

Summary

The MachineFunctionInfo here is a bit awkward because
WasmEHInfo is in the MachineFunction but handled from
the target code. Either everything should move into WebAssembly
or into the MachineFunction for MIR serialization.

[aheejin]

It's been a while and I don't remember why I put a pointer to MF as a member in WebAssemblyFunctionInfo, but if things work without it, it should be fine. But I don't understand some comments, about which I asked questions below.

I think the reason I put WasmEHInfo in MachineFunction is, Wasm is not only a target but an exception mode (even though it is only being used by WebAssembly target), and also WinEHInfo, which contains info for WinEH EH mode, is there too.

llvm/lib/Target/WebAssembly/WebAssemblyMachineFunctionInfo.cpp

31–35

What does this mean by "if WasmEHFuncInfo from the block reference"?

164

Why? Which link?

[pmatos]

llvm/lib/Target/WebAssembly/WebAssemblyMachineFunctionInfo.h

34

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.

[arsenm]

llvm/lib/Target/WebAssembly/WebAssemblyMachineFunctionInfo.cpp

31–35

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

164

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

llvm/lib/Target/WebAssembly/WebAssemblyMachineFunctionInfo.h

34

I purged this from all targets last time I posted this patch; this one was added back in at some point since then

[aheejin]

llvm/lib/Target/WebAssembly/WebAssemblyMachineFunctionInfo.cpp

31–35

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?

164

As I said, WinEHFuncInfo is also within MachineFunction. Also this WasmEHFuncInfo is not only for MIR handling. It is referenced throughout our compilation passes.

[arsenm]

llvm/lib/Target/WebAssembly/WebAssemblyMachineFunctionInfo.cpp

31–35

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

164

Right, it's a well spread out layering violation. The handling should be consistently in webassembly only, or consistently in the generic MachineFunction handling

[aheejin]

llvm/lib/Target/WebAssembly/WebAssemblyMachineFunctionInfo.cpp

31–35

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

164

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.

[arsenm]

Better comments and fail clone if EH info is used

[arsenm]

ff2b60bbcb28701c86b200b0c03439a2e6ef6b44 (except I dropped the MF check from the clone since it didn't actually work)

llvm/include/llvm/CodeGen/MachineFunction.h

class LLVM_EXTERNAL_VISIBILITY MachineFunction {
   MachineConstantPool *ConstantPool;
 
   // Keep track of jump tables for switch instructions
   MachineJumpTableInfo *JumpTableInfo;
 
   // Keep track of the function section.
   MCSection *Section = nullptr;
 
+  // Catchpad unwind destination info for wasm EH.
   // Keeps track of Wasm exception handling related data. This will be null for
   // functions that aren't using a wasm EH personality.
   WasmEHFuncInfo *WasmEHInfo = nullptr;
 
   // Keeps track of Windows exception handling related data. This will be null
   // for functions that aren't using a funclet-based EH personality.
   WinEHFuncInfo *WinEHInfo = nullptr;
 

llvm/lib/Target/WebAssembly/WebAssemblyMachineFunctionInfo.h

 
 namespace yaml {
 struct WebAssemblyFunctionInfo;
 }
 
 /// This class is derived from MachineFunctionInfo and contains private
 /// WebAssembly-specific information for each MachineFunction.
 class WebAssemblyFunctionInfo final : public MachineFunctionInfo {
-  const MachineFunction *MF;
-
   std::vector<MVT> Params;
   std::vector<MVT> Results;
   std::vector<MVT> Locals;
 
   /// A mapping from CodeGen vreg index to WebAssembly register number.
   std::vector<unsigned> WARegs;
 
   /// A mapping from CodeGen vreg index to a boolean value indicating whether
class WebAssemblyFunctionInfo final : public MachineFunctionInfo {
   unsigned FrameBaseVreg = -1U;
   // The local holding the frame base. This is either FP or SP
   // after WebAssemblyExplicitLocals
   unsigned FrameBaseLocal = -1U;
 
   // Function properties.
   bool CFGStackified = false;
 
-  // Catchpad unwind destination info for wasm EH.
-  WasmEHFuncInfo *WasmEHInfo = nullptr;
-
 public:
-  explicit WebAssemblyFunctionInfo(MachineFunction &MF_)
-      : MF(&MF_), WasmEHInfo(MF_.getWasmEHFuncInfo()) {}
+  explicit WebAssemblyFunctionInfo(MachineFunction &) {}
   ~WebAssemblyFunctionInfo() override;
 
   MachineFunctionInfo *
   clone(BumpPtrAllocator &Allocator, MachineFunction &DestMF,
         const DenseMap<MachineBasicBlock *, MachineBasicBlock *> &Src2DstMBB)
       const override;
 
-  const MachineFunction &getMachineFunction() const { return *MF; }
-
-  void initializeBaseYamlFields(const yaml::WebAssemblyFunctionInfo &YamlMFI);
+  void initializeBaseYamlFields(MachineFunction &MF,
+                                const yaml::WebAssemblyFunctionInfo &YamlMFI);
 
   void addParam(MVT VT) { Params.push_back(VT); }
   const std::vector<MVT> &getParams() const { return Params; }
 
   void addResult(MVT VT) { Results.push_back(VT); }
   const std::vector<MVT> &getResults() const { return Results; }
 
   void clearParamsAndResults() {
public:
   // For a given stackified WAReg, return the id number to print with push/pop.
   static unsigned getWARegStackId(unsigned Reg) {
     assert(Reg & INT32_MIN);
     return Reg & INT32_MAX;
   }
 
   bool isCFGStackified() const { return CFGStackified; }
   void setCFGStackified(bool Value = true) { CFGStackified = Value; }
-
-  WasmEHFuncInfo *getWasmEHFuncInfo() const { return WasmEHInfo; }
-  void setWasmEHFuncInfo(WasmEHFuncInfo *Info) { WasmEHInfo = Info; }
 };
 
 void computeLegalValueVTs(const WebAssemblyTargetLowering &TLI,
                           LLVMContext &Ctx, const DataLayout &DL, Type *Ty,
                           SmallVectorImpl<MVT> &ValueVTs);
 
 void computeLegalValueVTs(const Function &F, const TargetMachine &TM, Type *Ty,
                           SmallVectorImpl<MVT> &ValueVTs);
struct WebAssemblyFunctionInfo final : public yaml::MachineFunctionInfo {
   std::vector<FlowStringValue> Params;
   std::vector<FlowStringValue> Results;
   bool CFGStackified = false;
   // The same as WasmEHFuncInfo's SrcToUnwindDest, but stored in the mapping of
   // BB numbers
   BBNumberMap SrcToUnwindDest;
 
   WebAssemblyFunctionInfo() = default;
-  WebAssemblyFunctionInfo(const llvm::WebAssemblyFunctionInfo &MFI);
+  WebAssemblyFunctionInfo(const llvm::MachineFunction &MF,
+                          const llvm::WebAssemblyFunctionInfo &MFI);
 
   void mappingImpl(yaml::IO &YamlIO) override;
   ~WebAssemblyFunctionInfo() = default;
 };
 
 template <> struct MappingTraits<WebAssemblyFunctionInfo> {
   static void mapping(IO &YamlIO, WebAssemblyFunctionInfo &MFI) {
     YamlIO.mapOptional("params", MFI.Params, std::vector<FlowStringValue>());

llvm/lib/Target/WebAssembly/WebAssemblyMachineFunctionInfo.cpp

 using namespace llvm;
 
 WebAssemblyFunctionInfo::~WebAssemblyFunctionInfo() = default; // anchor.
 
 MachineFunctionInfo *WebAssemblyFunctionInfo::clone(
     BumpPtrAllocator &Allocator, MachineFunction &DestMF,
     const DenseMap<MachineBasicBlock *, MachineBasicBlock *> &Src2DstMBB)
     const {
-  WebAssemblyFunctionInfo *Clone =
-      DestMF.cloneInfo<WebAssemblyFunctionInfo>(*this);
-  Clone->MF = &DestMF;
-  return Clone;
+  // TODO: Implement cloning for WasmEHFuncInfo. This will have invalid block
+  // references.
+  if (!MF.getWasmEHFuncInfo())
+    return nullptr;
+  return DestMF.cloneInfo<WebAssemblyFunctionInfo>(*this);
 }
 
 void WebAssemblyFunctionInfo::initWARegs(MachineRegisterInfo &MRI) {
   assert(WARegs.empty());
   unsigned Reg = UnusedReg;
   WARegs.resize(MRI.getNumVirtRegs(), Reg);
 }
 
llvm::signatureFromMVTs(const SmallVectorImpl<MVT> &Results,
                         const SmallVectorImpl<MVT> &Params) {
   auto Sig = std::make_unique<wasm::WasmSignature>();
   valTypesFromMVTs(Results, Sig->Returns);
   valTypesFromMVTs(Params, Sig->Params);
   return Sig;
 }
 
 yaml::WebAssemblyFunctionInfo::WebAssemblyFunctionInfo(
-    const llvm::WebAssemblyFunctionInfo &MFI)
+    const llvm::MachineFunction &MF, const llvm::WebAssemblyFunctionInfo &MFI)
     : CFGStackified(MFI.isCFGStackified()) {
-  auto *EHInfo = MFI.getWasmEHFuncInfo();
-  const llvm::MachineFunction &MF = MFI.getMachineFunction();
-
   for (auto VT : MFI.getParams())
     Params.push_back(EVT(VT).getEVTString());
   for (auto VT : MFI.getResults())
     Results.push_back(EVT(VT).getEVTString());
 
   //  MFI.getWasmEHFuncInfo() is non-null only for functions with the
   //  personality function.
-  if (EHInfo) {
+
+  if (auto *EHInfo = MF.getWasmEHFuncInfo()) {
     // SrcToUnwindDest can contain stale mappings in case BBs are removed in
     // optimizations, in case, for example, they are unreachable. We should not
     // include their info.
     SmallPtrSet<const MachineBasicBlock *, 16> MBBs;
     for (const auto &MBB : MF)
       MBBs.insert(&MBB);
     for (auto KV : EHInfo->SrcToUnwindDest) {
       auto *SrcBB = KV.first.get<MachineBasicBlock *>();
       auto *DestBB = KV.second.get<MachineBasicBlock *>();
       if (MBBs.count(SrcBB) && MBBs.count(DestBB))
         SrcToUnwindDest[SrcBB->getNumber()] = DestBB->getNumber();
     }
   }
 }
 
 void yaml::WebAssemblyFunctionInfo::mappingImpl(yaml::IO &YamlIO) {
   MappingTraits<WebAssemblyFunctionInfo>::mapping(YamlIO, *this);
 }
 
 void WebAssemblyFunctionInfo::initializeBaseYamlFields(
-    const yaml::WebAssemblyFunctionInfo &YamlMFI) {
+    MachineFunction &MF, const yaml::WebAssemblyFunctionInfo &YamlMFI) {
   CFGStackified = YamlMFI.CFGStackified;
   for (auto VT : YamlMFI.Params)
     addParam(WebAssembly::parseMVT(VT.Value));
   for (auto VT : YamlMFI.Results)
     addResult(WebAssembly::parseMVT(VT.Value));
-  if (WasmEHInfo) {
+
+  // FIXME: WasmEHInfo is defined in the MachineFunction, but serialized
+  // here. Either WasmEHInfo should be moved out of MachineFunction, or the
+  // serialization handling should be moved to MachineFunction.
+  if (WasmEHFuncInfo *WasmEHInfo = MF.getWasmEHFuncInfo()) {
     for (auto KV : YamlMFI.SrcToUnwindDest)
-      WasmEHInfo->setUnwindDest(MF->getBlockNumbered(KV.first),
-                                MF->getBlockNumbered(KV.second));
+      WasmEHInfo->setUnwindDest(MF.getBlockNumbered(KV.first),
+                                MF.getBlockNumbered(KV.second));
   }
 }

llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp

 yaml::MachineFunctionInfo *
 WebAssemblyTargetMachine::createDefaultFuncInfoYAML() const {
   return new yaml::WebAssemblyFunctionInfo();
 }
 
 yaml::MachineFunctionInfo *WebAssemblyTargetMachine::convertFuncInfoToYAML(
     const MachineFunction &MF) const {
   const auto *MFI = MF.getInfo<WebAssemblyFunctionInfo>();
-  return new yaml::WebAssemblyFunctionInfo(*MFI);
+  return new yaml::WebAssemblyFunctionInfo(MF, *MFI);
 }
 
 bool WebAssemblyTargetMachine::parseMachineFunctionInfo(
     const yaml::MachineFunctionInfo &MFI, PerFunctionMIParsingState &PFS,
     SMDiagnostic &Error, SMRange &SourceRange) const {
   const auto &YamlMFI = static_cast<const yaml::WebAssemblyFunctionInfo &>(MFI);
   MachineFunction &MF = PFS.MF;
-  MF.getInfo<WebAssemblyFunctionInfo>()->initializeBaseYamlFields(YamlMFI);
+  MF.getInfo<WebAssemblyFunctionInfo>()->initializeBaseYamlFields(MF, YamlMFI);
   return false;
 }