Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sroa: Lift restriction that all_same optimization must give SSAValue #52338

Merged
merged 3 commits into from
Dec 7, 2023

Conversation

Keno
Copy link
Member

@Keno Keno commented Nov 28, 2023

This restriction has been in there since this code was added in #44557. Unfortunately, I can't tell from the commit history why this restriction was added. It's possible that it was trying to avoid putting things into statement position that were not allowed in the phi block, but we have cleaned that up since (#50308 and related), so let's see if this restriction is still required, since I was seeing some suboptimial optimization results because of this.

@Keno
Copy link
Member Author

Keno commented Nov 28, 2023

@nanosoldier runtests()

Copy link
Member

@aviatesk aviatesk left a comment

Choose a reason for hiding this comment

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

The base test suite seems to be successful on this PR.

base/compiler/ssair/inlining.jl Outdated Show resolved Hide resolved
@nanosoldier
Copy link
Collaborator

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

Keno and others added 2 commits December 1, 2023 09:22
This restriction has been in there since this code was added in #44557.
Unfortunately, I can't tell from the commit history why this restriction
was added. It's possible that it was trying to avoid putting things into
statement position that were not allowed in the phi block, but we have
cleaned that up since (#50308 and related), so let's see if this restriction
is still required, since I was seeing some suboptimial optimization results
because of this.
@aviatesk aviatesk force-pushed the kf/sroarelaxallsame branch from 5179b1c to 74b8b6a Compare December 1, 2023 00:26
@aviatesk
Copy link
Member

aviatesk commented Dec 1, 2023

@nanosoldier runtests(["GridapGmsh", "FlexPlan", "PowerModelsWildfire", "SegregatedVMSSolver", "CbaOPF", "Krotov", "TopoPlots", "LIBLINEAR", "Gtk4", "GraphViz", "FileWatching"], vs = ":master")

@Keno
Copy link
Member Author

Keno commented Dec 1, 2023

FWIW, I found the bug that showed up on PkgEval, but it unraveled a whole thread, so standby for that

@nanosoldier
Copy link
Collaborator

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

@Keno
Copy link
Member Author

Keno commented Dec 2, 2023

Sanity check to make sure I actually fixed the bug:
@nanosoldier runtests(["PowerAnalytics", "SetProg", "GraphViz", "Gtk4", "REopt", "Biofilm", "NonconvexBayesian", "FourierTools", "SciMLBase", "Tidier", "GridapGmsh", "Scruff", "TSML", "MRISimulation", "PlantRayTracer", "ReliabilityOptimization", "PowerModelsWildfire", "ImageFeatures", "ObservableCortex", "XGPaint", "MCPTrajectoryGameSolver", "AiidaDFTK", "ONSAS", "FileWatching", "GridVisualize", "PowerModelsDistribution", "Polynomials4ML", "FlexPlan", "CbaOPF", "QUnfold", "SensitivityRankCondition", "CombinatorialSpaces", "AlgamesDriving", "QuartetNetworkGoodnessFit", "DynamicMovementPrimitives", "MiseEnPage", "CausalForest", "EditorsRepo"])
However, I'd like to run a full PkgEval afterwards anyway, because I did some fairly significant code re-arragements, so I want to make sure nothing else broke.

@nanosoldier
Copy link
Collaborator

The package evaluation job you requested has completed - no new issues were detected.
The full report is available.

@Keno
Copy link
Member Author

Keno commented Dec 2, 2023

@nanosoldier runtests()

@nanosoldier
Copy link
Collaborator

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

@Keno Keno force-pushed the kf/sroarelaxallsame branch from ae2d3cd to 05fbff4 Compare December 7, 2023 00:14
Although the path that created the NewSSAValue in the PkgEval case
shouldn't really be there, so also add a small optimization that
renames ssa values more aggressively, so that we can actually test
this.
@Keno Keno force-pushed the kf/sroarelaxallsame branch from 05fbff4 to 50f64d0 Compare December 7, 2023 00:23
@Keno
Copy link
Member Author

Keno commented Dec 7, 2023

@nanosoldier runtests(["StenoGraphs", "ModiaPlot_WGLMakie", "BSeries", "HmtArchive", "PowerModelsDistribution", "MLJTestIntegration", "CompressiveLearning", "OptimalTransmissionRouting", "TaylorModels", "Pesto", "PowerModelsACDC", "ConstraintTrees", "DiscretePIDs", "Consensus", "EMpht", "FlowAtlas", "Syslogs", "GameTheory", "ReliabilityOptimization", "PALEOmodel", "GeneralizedRouwenhorst", "MiseEnPage", "AiidaDFTK", "Biplots", "HashCode2014", "KelvinletsImage", "ChaosTools", "FractalDimensions", "MRICoilSensitivities", "MRISimulation", "QuantumDynamics", "Oxygen", "PlantMeteo", "GeoParams", "GlobalSensitivity", "LibTrixi", "SMLMMetrics", "FMIImport", "GIFImages", "ReferenceTests", "MixedModels", "Permanents", "Yunir", "MINDFulMakie", "MadNLP", "AutoBZCore"])

@nanosoldier
Copy link
Collaborator

The package evaluation job you requested has completed - no new issues were detected.
The full report is available.

@Keno Keno merged commit 0ba0157 into master Dec 7, 2023
6 of 8 checks passed
@Keno Keno deleted the kf/sroarelaxallsame branch December 7, 2023 17:06
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