Page MenuHomePhabricator

Remove gc.roots performCustomLowering

Authored by reames on Jan 27 2015, 6:02 PM.



This is a refactoring to restructure the single user of performCustomLowering as a specific lowering pass and remove the custom lowering hook entirely.

Before this change, the LowerIntrinsics pass (note to self: rename!) was essentially acting as a pass manager, but without being structured in terms of passes. Instead, it proxied calls to a set of GCStrategies internally. This adds a lot of conceptual complexity (i.e. GCStrategies are stateful!) for very little benefit. Since there's been interest in keeping the ShadowStackGC working, I extracting it's custom lowering pass into a dedicated pass and just added that to the pass order. It will only run for functions which opt-in to that gc.

I wasn't able to find an easy way to preserve the runtime registration of custom lowering functionality. Given that no user of this exists that I'm aware of, I made the choice to just remove that. If someone really cares, we can look at restoring it via dynamic pass registration in the future.

Note that despite the large diff, none of the lowering code actual changes. I added the framing needed to make it a pass and rename the class, but that's it.

Diff Detail


Event Timeline

reames updated this revision to Diff 18871.Jan 27 2015, 6:02 PM
reames retitled this revision from to Remove gc.roots performCustomLowering.
reames updated this object.
reames edited the test plan for this revision. (Show Details)
reames added reviewers: whitequark, mjacob, artagnon.
reames added a subscriber: Unknown Object (MLST).
whitequark accepted this revision.Jan 27 2015, 6:26 PM
whitequark edited edge metadata.


This revision is now accepted and ready to land.Jan 27 2015, 6:26 PM
mjacob edited edge metadata.Jan 28 2015, 4:22 AM

I like the direction of this patch. Probably medium-term the feature to add custom lowering passes should be re-added, but I'm fine if we ignore that for now.

751 ↗(On Diff #18871)

I think these three points can stay in because they're still valid.

520 ↗(On Diff #18871)


119 ↗(On Diff #18871)

This comment isn't true since there's no initializeCustomLowering anymore.

234 ↗(On Diff #18871)

I think it's now possible to add ModulePasses to the llc pass pipeline, so changing this pass to be a ModulePass should be possible.

Responding to comments. (Those not specifically responded to will be fixed before submission.)

751 ↗(On Diff #18871)

They're also covered elsewhere in the same file already.

234 ↗(On Diff #18871)

I think this comment is just stale. Given I didn't touch any of this code, I'll leave it as is for now.

This revision was automatically updated to reflect the committed changes.