Skip to content

Decouple and refactor DynFlags in CmmToLlvm

doyougnu requested to merge doyougnu/ghc:wip/CmmToLlvm-decouple-dynflags into master

What

Adds two modules:

  • GHC.CmmToLlvm.Config -- module that defines the LCGConfig type (The DynFlags for CmmToLlvm pass, read as Llvm Code Gen Config)
  • GHC.Driver.Config.CmmToLlvm -- module that defines the initialization function, i.e., initLCGConfig :: DynFlags -> LCGConfig this will be the site of the future handshake
  • Absorb LlvmOpts into new type LCGConfig

Removes all references to DynFlags in the CmmToLlvm pass, swapping out these calls to a new record LCGConfig which is defined to hold only what CmmToLlvm needs in order to operate.

[Edit]: refactor llvm code generation to ensure GHC.Data.FastString length is calculated at compile time. See this comment and commit 89718b7c

Why

  • see issue #20730, specifically this comment
  • This MR implements Goal 1: Isolation for CmmToLlvm, Goal 2 and 3 left to do.

Other things and where I'm unsure

  • I've changed a lot of code that was only using DynFlags for error messages via panic or pprPanic, most of these changes swap lines like showSDoc dflags <something> to renderWithContext (lcgContext cfg) <something>, where lcgContext is defined initialized to initSDocContext dflags (PprCode CStyle). This is different then what is on master, on master showSDoc uses a default context: showSDoc dflags sdoc = renderWithContext (initSDocContext dflags defaultUserStyle) sdoc.

So:

  1. I am not sure if this is a problematic change, please advise
  2. That showSDoc takes DynFlags at all seems quite bad to me. I think that this is actually a driver of the coupling of DynFlags to a bunch of code throughout the compiler. Imagine you're writing some code then you hit a point and think "Oh I need to panic here", and then see "oh panic takes a String", and then you lookup how to make your SDoc and then show it with showSDoc and we are off to the races.
  3. The majority of changes are in CmmToLlvm (obviously) but I'm unsure how to test and verify that these changes are sound (i.e., I haven't introduced bugs). Are there tests that specficially test -fllvm? Or, what is the best way to verify (for example CI fails on Windows, but passes everywhere else, is this expected?)

Todos

Once these todos are closed I'll mark as ready to merge:

  • confirmation that (1) is not problematic or has been addressed
  • confirmation that (3) has been addressed and the MR has been sufficiently tested before merge.

[Edit]: Todos closed, see this comment

Edited by doyougnu

Merge request reports