Skip to content

Runner cleanup#449

Merged
katrinafyi merged 40 commits intomainfrom
runner-cleanup
Sep 15, 2025
Merged

Runner cleanup#449
katrinafyi merged 40 commits intomainfrom
runner-cleanup

Conversation

@j-tobler
Copy link
Contributor

@j-tobler j-tobler commented Jun 6, 2025

Some initial progress towards cleaning up RunUtils.

Created classes Transform, StaticAnalysis and AnalysisManager. Refactored doCleanup, prepareForTranslation, generateRelyGuaranteeConditions, and generateFunctionSummaries to use the Transform class.

@j-tobler j-tobler requested a review from ailrst June 6, 2025 01:22
@ailrst
Copy link
Contributor

ailrst commented Jun 6, 2025

./mill scalafmt.reformat will run the autoformatter

Copy link
Member

@katrinafyi katrinafyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems fine, it's very good to unify the dumping and logging things.

comments are more for future work

Comment on lines +25 to +26
if memo.isEmpty then memo = Some(analysis(AnalysisManager.this))
memo.get
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use pattern match

Comment on lines 55 to 56
def getStripUnreachableFunctionsTransform(depth: Int): Transform =
SingleTransform(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i dont really like the need to wrap parametrised transforms in a get method. maybe the Transform class can be changed to take an additional custom parameter.

Comment on lines +11 to +28
trait Log {
def dump(ctx: IRContext, transformName: String): Unit
}

// dumps a blockgraph log
case class BlockgraphLog(filenamePrefix: String) extends Log {
def dump(ctx: IRContext, transformName: String): Unit =
DebugDumpIRLogger.writeToFile(
File(s"${filenamePrefix}_blockgraph-${transformName}.dot"),
dotBlockGraph(ctx.program.mainProcedure)
)
}

// dumps an IR log
case class IrLog(filenamePrefix: String) extends Log {
def dump(ctx: IRContext, transformName: String): Unit =
DebugDumpIRLogger.writeToFile(File(s"${filenamePrefix}_il-${transformName}.il"), pp_prog(ctx.program))
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the point of this? these could just be methods surely

Comment on lines 61 to 80
// executes the transform with any modifications or book-keeping specified by the given config
def apply(ctx: IRContext, man: AnalysisManager, config: TransformConfig = emptyConfig): Unit = {
if (config.disabled.contains(this)) return
if (notice != "") then Logger.info(s"[!] ${notice}")
if (man.program ne ctx.program) {
// the program we are transforming should be the same one for which the analysis results were produced
throw new RuntimeException(
s"Transform '$name' was passed an AnalysisManager of an IR Program with a different " +
s"reference value than the program being transformed."
)
}
val maybeLogs = config.dumpLogs.get(this)
maybeLogs.foreach(_.foreach(_.dump(ctx, s"before-$name")))
timer.checkPoint("start")
val invalidation = transform(ctx, man, config) // run the actual transform and get the analysis results to clobber
timer.checkPoint("end")
postRun(ctx) // runs after performance checks, and before logging
maybeLogs.foreach(_.foreach(_.dump(ctx, s"after-$name")))
man.invalidate(invalidation) // clobber the specified analysis results
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this method belongs more in the AnalysisManager, but shrug

Comment on lines 85 to 89
case class SingleTransform(
name: String,
implementation: (ctx: IRContext, man: AnalysisManager) => man.Invalidation,
notice: String = ""
) extends Transform {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

single transform could be done as a special case of transform batch

@github-actions
Copy link
Contributor

github-actions bot commented Aug 14, 2025

scalafmt check passed.

last updated at 61680f9 (logs).

@j-tobler
Copy link
Contributor Author

This most recent merge had some pretty significant conflicts.

Fortunately, most of these were due to @katrinafyi's changes to the IRLoading object, which @ailrst moved to a new file in the feature branch. I resolved these by overwriting the new file with Kait's changes.

Another was due to the new loop invariant generation pass, which I resolved by creating a new reducibleLoops function in AnalysePipelineMRA.scala, effectively re-creating the function added to the now-removed StaticAnalysis object in RunUtils.

One important change that I'm not sure about is the tests. Particularly, I removed some tests from IntervalDSATest.scala in an attempt to capture the latest changes made to the file. I'm not sure if this was correct and it would be good to check this with the author of the tests.

To ensure the refactor preserves the old behaviour, I loosely combed through the prepareForTranslation and doCleanup transforms and didn't find any discrepancies with the original functions in RunUtils. The doSimplify transform is still under development in Simp.scala.

@katrinafyi katrinafyi merged commit 518a035 into main Sep 15, 2025
12 checks passed
@katrinafyi katrinafyi deleted the runner-cleanup branch September 15, 2025 07:16
katrinafyi pushed a commit that referenced this pull request Sep 16, 2025
Some initial progress towards cleaning up RunUtils.

Created classes Transform, StaticAnalysis and AnalysisManager.
Refactored doCleanup, prepareForTranslation,
generateRelyGuaranteeConditions, and generateFunctionSummaries to use
the Transform class.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants