Page MenuHomePhabricator

Keep Optimization Remark Yaml in NewPM
ClosedPublic

Authored by lenary on Aug 18 2017, 5:21 PM.

Details

Summary

The New Pass Manager infrastructure was forgetting to keep around the optimization remark yaml file that the compiler might have been producing. This meant setting the option to '-' for stdout worked, but setting it to a filename didn't give file output (presumably it was deleted because compilation didn't explicitly keep it). This change just ensures that the file is kept if compilation succeeds.

So far I have updated one of the optimization remark output tests to add a version with the new pass manager. It is my intention for this patch to also include changes to all tests that use -opt-remark-output= but I wanted to get the code patch ready for review while I was making all those changes.

Fixes https://bugs.llvm.org/show_bug.cgi?id=33951

Diff Detail

Repository
rL LLVM

Event Timeline

lenary created this revision.Aug 18 2017, 5:21 PM
anemet accepted this revision.Aug 18 2017, 10:24 PM

LGTM, thanks!

tools/opt/NewPMDriver.cpp
173 ↗(On Diff #111775)

I know that I called this YamlFile in opt.cpp but that's really unfortunate in retrospect. Can you please call this something like OptRemarksFile or something?

This revision is now accepted and ready to land.Aug 18 2017, 10:24 PM
lenary updated this revision to Diff 111807.Aug 19 2017, 1:34 AM

Better Patch for opt remark file issue, undoing changes to new PM interface, and renaming variables as requested.

chandlerc requested changes to this revision.Aug 19 2017, 1:41 AM
chandlerc added a subscriber: chandlerc.
chandlerc added inline comments.
tools/opt/opt.cpp
547–548 ↗(On Diff #111807)

For Out and ThinLinkOut, we do the ->keep() call inside runPassPipeline. I would keep this consistent with the other files.

This revision now requires changes to proceed.Aug 19 2017, 1:41 AM
lenary updated this revision to Diff 111828.Aug 19 2017, 8:36 AM
lenary edited edge metadata.
lenary marked an inline comment as done.

Move OptRemarkFile back into runPassPipeline

lenary updated this revision to Diff 111836.Aug 19 2017, 10:04 AM

All remark tests now go through both the old and new pass managers.

chandlerc accepted this revision.Aug 19 2017, 4:39 PM

LGTM. Andrew already said LGTM, so feel free to land.

This revision is now accepted and ready to land.Aug 19 2017, 4:39 PM
This revision was automatically updated to reflect the committed changes.