Page MenuHomePhabricator

[WebAssembly] Change WebAssemblyMCLowerPrePass to ModulePass
ClosedPublic

Authored by aardappel on Sep 2 2021, 3:45 PM.

Details

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

Diff Detail

Event Timeline

aardappel created this revision.Sep 2 2021, 3:45 PM
aardappel requested review of this revision.Sep 2 2021, 3:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 2 2021, 3:45 PM
keithw added a subscriber: keithw.Sep 2 2021, 3:56 PM

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.

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?

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.

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

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.

@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) ?

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

Yes, it must always be run just before AsmPrinter.

This revision was not accepted when it landed; it landed in state Needs Review.Sep 8 2021, 10:48 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.

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.

LGTM, for the record.