[WebAssembly] Change WebAssemblyMCLowerPrePass to ModulePass

[aardappel]

Summary

It was a FunctionPass before, which subverted its purpose to collect ALL symbols before MCLowering, depending on how LLVM schedules function passes.
Fixes https://bugs.llvm.org/show_bug.cgi?id=51555

[dschuff]

Does this actually work? I thought there was no such thing as a MachineModulePass and that backend passes couldn't be module passes. I wonder if that has changed.

[aardappel]

Well.. yes :) The code runs, and unlike before, causes all symbols for all functions to be collected before MCLowering.
It passes all existing tests that expect __stack_pointer, and the new test fails without this patch because the .globaltype is not emitted, much like in the reported bug.
Do you recommend anyone to add to this review to see if I am not committing a grave LLVM-no-no?

[aardappel]

Also there is no MachineModule.. MachineModuleInfo is the closest thing.. that may explain why there's no MachineModulePass. The code to obtain MachineModuleInfo is a bit clumsy though, but I've seen other passes use it this way too.

[dschuff]

@aeubanks is probably the pass manager expert, and my knowledge is very likely out of date.

[aeubanks]

IIUC, you're trying to make this pass run on all functions before some later function pass. A cleaner way is to add a barrier pass either after this pass or before AsmPrinter (or whatever pass you want) via createBarrierNoopPass(). That will force all function passes to run on all functions before the barrier pass, since the pass manager can't schedule function passes through a module pass so it creates two function pass managers, one before and one after the barrier module pass.

[aardappel]

@aeubanks looked up the use of this pass, and the comments next to its usage say:

// FIXME: The BarrierNoopPass is a HACK! The inliner pass above implicitly
// creates a CGSCC pass manager, but we don't want to add extensions into
// that pass manager. To prevent this we insert a no-op module pass to reset
// the pass manager to get the same behavior as EP_OptimizerLast in non-O0
// builds. The function merging pass is

Also this pass seems to only be declared and used as part of the "IPO transformations library", does it make sense for Wasm to depend on that? Or should I be making my own empty barrier pass? Is that still "cleaner" than what I have done here (naturally make it part of the semantics of the pass that actually does the work) ?

[aeubanks]

if this pass must always be run without being interleaved with other passes then making it a module pass makes sense
otherwise I'd use the barrier pass to explicitly control scheduling

[aardappel]

Yes, it must always be run just before AsmPrinter.

[aardappel]

Apologies for pushing this to main, I did not see there was no "accepted" status yet, though did check with @dschuff offline that he was ok with this patch as-is.

[dschuff]

LGTM, for the record.

llvm/lib/Target/WebAssembly/WebAssembly.h

 FunctionPass *createWebAssemblyLateEHPrepare();
 FunctionPass *createWebAssemblyCFGSort();
 FunctionPass *createWebAssemblyCFGStackify();
 FunctionPass *createWebAssemblyExplicitLocals();
 FunctionPass *createWebAssemblyLowerBrUnless();
 FunctionPass *createWebAssemblyRegNumbering();
 FunctionPass *createWebAssemblyDebugFixup();
 FunctionPass *createWebAssemblyPeephole();
-FunctionPass *createWebAssemblyMCLowerPrePass();
+ModulePass *createWebAssemblyMCLowerPrePass();
 
 // PassRegistry initialization declarations.
 void initializeWebAssemblyAddMissingPrototypesPass(PassRegistry &);
 void initializeWebAssemblyLowerEmscriptenEHSjLjPass(PassRegistry &);
 void initializeLowerGlobalDtorsPass(PassRegistry &);
 void initializeFixFunctionBitcastsPass(PassRegistry &);
 void initializeOptimizeReturnedPass(PassRegistry &);
 void initializeWebAssemblyArgumentMovePass(PassRegistry &);

llvm/lib/Target/WebAssembly/WebAssemblyMCLowerPrePass.cpp

 #include "llvm/CodeGen/Passes.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/raw_ostream.h"
 using namespace llvm;
 
 #define DEBUG_TYPE "wasm-mclower-prepass"
 
 namespace {
-class WebAssemblyMCLowerPrePass final : public MachineFunctionPass {
+class WebAssemblyMCLowerPrePass final : public ModulePass {
   StringRef getPassName() const override {
     return "WebAssembly MC Lower Pre Pass";
   }
 
   void getAnalysisUsage(AnalysisUsage &AU) const override {
     AU.setPreservesCFG();
-    MachineFunctionPass::getAnalysisUsage(AU);
+    ModulePass::getAnalysisUsage(AU);
   }
 
-  bool runOnMachineFunction(MachineFunction &MF) override;
+  bool runOnModule(Module &M) override;
 
 public:
   static char ID; // Pass identification, replacement for typeid
-  WebAssemblyMCLowerPrePass() : MachineFunctionPass(ID) {}
+  WebAssemblyMCLowerPrePass() : ModulePass(ID) {}
 };
 } // end anonymous namespace
 
 char WebAssemblyMCLowerPrePass::ID = 0;
 INITIALIZE_PASS(
     WebAssemblyMCLowerPrePass, DEBUG_TYPE,
     "Collects information ahead of time for MC lowering",
     false, false)
 
-FunctionPass *llvm::createWebAssemblyMCLowerPrePass() {
+ModulePass *llvm::createWebAssemblyMCLowerPrePass() {
   return new WebAssemblyMCLowerPrePass();
 }
 
-bool WebAssemblyMCLowerPrePass::runOnMachineFunction(MachineFunction &MF) {
-  LLVM_DEBUG(dbgs() << "********** MC Lower Pre Pass **********\n"
-                       "********** Function: "
-                    << MF.getName() << '\n');
+// NOTE: this is a ModulePass since we need to enforce that this code has run
+// for all functions before AsmPrinter. If this way of doing things is ever
+// suboptimal, we could opt to make it a MachineFunctionPass and instead use
+// something like createBarrierNoopPass() to enforce ordering.
+bool WebAssemblyMCLowerPrePass::runOnModule(Module &M) {
+  auto *MMIWP = getAnalysisIfAvailable<MachineModuleInfoWrapperPass>();
+  if (!MMIWP)
+    return true;
 
-  MachineModuleInfo &MMI = MF.getMMI();
+  MachineModuleInfo &MMI = MMIWP->getMMI();
   MachineModuleInfoWasm &MMIW = MMI.getObjFileInfo<MachineModuleInfoWasm>();
 
-  for (MachineBasicBlock &MBB : MF) {
+  for (Function &F : M) {
+    MachineFunction *MF = MMI.getMachineFunction(F);
+    if (!MF)
+      continue;
+
+    LLVM_DEBUG(dbgs() << "********** MC Lower Pre Pass **********\n"
+                         "********** Function: "
+                      << MF->getName() << '\n');
+
+    for (MachineBasicBlock &MBB : *MF) {
       for (auto &MI : MBB) {
         // FIXME: what should all be filtered out beyond these?
         if (MI.isDebugInstr() || MI.isInlineAsm())
           continue;
         for (MachineOperand &MO : MI.uses()) {
           if (MO.isSymbol()) {
             MMIW.MachineSymbolsUsed.insert(MO.getSymbolName());
           }
         }
       }
     }
-
+  }
   return true;
 }

llvm/test/MC/WebAssembly/stack-ptr-mclower.ll

+; RUN: llc --mtriple=wasm32-unknown-unknown -asm-verbose=false %s -o - | FileCheck --check-prefixes CHECK -DPTR=i32 %s
+; RUN: llc --mtriple=wasm64-unknown-unknown -asm-verbose=false %s -o - | FileCheck --check-prefixes CHECK -DPTR=i64 %s
+
+; empty function that does not use stack, to check WebAssemblyMCLowerPrePass
+; works correctly.
+define hidden void @bar() #0 {
+  ret void
+}
+
+; Function that uses explict stack, and should generate a reference to
+; __stack_pointer, along with the corresponding reloction entry.
+define hidden void @foo() #0 {
+entry:
+  alloca i32, align 4
+  ret void
+}
+
+; CHECK:              .text
+; CHECK-NEXT:         .file   "stack-ptr-mclower.ll"
+; CHECK-NEXT:         .section        .text.bar,"",@
+; CHECK-NEXT:         .hidden bar
+; CHECK-NEXT:         .globl  bar
+; CHECK-NEXT:         .globaltype     __stack_pointer, [[PTR]]
+; CHECK-NEXT:         .type   bar,@function
+; CHECK-NEXT: bar:
+; CHECK-NEXT:         .functype       bar () -> ()
+; CHECK-NEXT:         end_function
+; CHECK-NEXT: .Lfunc_end0:
+; CHECK-NEXT:         .size   bar, .Lfunc_end0-bar
+
+; CHECK:              .section        .text.foo,"",@
+; CHECK-NEXT:         .hidden foo
+; CHECK-NEXT:         .globl  foo
+; CHECK-NEXT:         .type   foo,@function
+; CHECK-NEXT: foo:
+; CHECK-NEXT:         .functype       foo () -> ()
+; CHECK-NEXT:         global.get      __stack_pointer
+; CHECK-NEXT:         [[PTR]].const       16
+; CHECK-NEXT:         [[PTR]].sub
+; CHECK-NEXT:         drop
+; CHECK-NEXT:         end_function
+; CHECK-NEXT: .Lfunc_end1:
+; CHECK-NEXT:         .size   foo, .Lfunc_end1-foo