Skip to content
GitLab
Projects Groups Snippets
  • /
  • Help
    • Help
    • Support
    • Community forum
    • Submit feedback
  • Sign in / Register
  • GHC GHC
  • Project information
    • Project information
    • Activity
    • Labels
    • Members
  • Repository
    • Repository
    • Files
    • Commits
    • Branches
    • Tags
    • Contributors
    • Graph
    • Compare
    • Locked Files
  • Issues 5,346
    • Issues 5,346
    • List
    • Boards
    • Service Desk
    • Milestones
    • Iterations
  • Merge requests 572
    • Merge requests 572
  • CI/CD
    • CI/CD
    • Pipelines
    • Jobs
    • Schedules
    • Test Cases
  • Deployments
    • Deployments
    • Releases
  • Analytics
    • Analytics
    • Value stream
    • CI/CD
    • Code review
    • Insights
    • Issue
    • Repository
  • Wiki
    • Wiki
  • Snippets
    • Snippets
  • Activity
  • Graph
  • Create a new issue
  • Jobs
  • Commits
  • Issue Boards
Collapse sidebar
  • Glasgow Haskell CompilerGlasgow Haskell Compiler
  • GHCGHC
  • Merge requests
  • !7158

Decouple and refactor DynFlags in CmmToLlvm

  • Review changes

  • Download
  • Email patches
  • Plain diff
Closed doyougnu requested to merge doyougnu/ghc:wip/CmmToLlvm-decouple-dynflags into master Dec 10, 2021
  • Overview 15
  • Commits 5
  • Pipelines 5
  • Changes 13

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 Dec 13, 2021 by doyougnu
Assignee
Assign to
Reviewers
Request review from
Time tracking
Source branch: wip/CmmToLlvm-decouple-dynflags