Page MenuHomePhabricator

NFC: Simplify O1 pass pipeline construction.
AcceptedPublic

Authored by echristo on May 4 2020, 6:42 PM.

Details

Reviewers
chandlerc
Summary

Improve readability and make it easier to simplify the pipeline moving forward.

Pull O1 pass pipeline out into a separate function and simplify
buildFunctionSimplificationPipeline accordingly.

Diff Detail

Event Timeline

echristo created this revision.May 4 2020, 6:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 4 2020, 6:42 PM

As much as I'm not thrilled about the duplication, the number of comments I have below about the exact O1 pipeline sure make it seem valuable. Unsure how you feel about addressing these here, later, etc.

llvm/lib/Passes/PassBuilder.cpp
435

Mapbe pull to the function comment? Doesn't seem especially relevant here given the other SimplifyCFGPass runs.

436

I find the sequence simplify-cfg -> instcombine -> libcall-shrink-wrap -> simplify-cfg a bit odd... Does the shrink wrap pass require running after instcombine? If not, maybe do libcall-shrink-wrap -> simplify-cfg -> instcombine?

444–451

Maybe O1 is a place to go ahead and addres what we can of this?

515–518

I'd skip this one at O1, FWIW.

we're going to run it again just after this. Also seems a bit redundant to run BDCE right before ADCE... but you already have a TODO down there.

As much as I'm not thrilled about the duplication, the number of comments I have below about the exact O1 pipeline sure make it seem valuable. Unsure how you feel about addressing these here, later, etc.

Later :) I wanted to get the actual separation as a noop and then make actual substantial changes as I could do them.

I have a couple more pending patches as well. That said, appreciate the specific suggestions - I think the new format makes it much easier to really see what's going on here (and yes, lots of room for cleanup/changes/investigation).

-eric

chandlerc accepted this revision.May 12 2020, 11:08 PM

LGTM, but one of these I think should be addressed here.

Also, definitely want to see the rest of these. W/o TODOs, always worried about losing track...

llvm/lib/Passes/PassBuilder.cpp
435

FWIW, I think this is the one I'd go ahead and do in this patch -- it seems specifically relevant to the extraction of the pipeline.

This revision is now accepted and ready to land.May 12 2020, 11:08 PM
echristo marked an inline comment as done.May 29 2020, 7:27 PM

Thanks. I'll go ahead and commit after the loop unrolling optimization patch.

llvm/lib/Passes/PassBuilder.cpp
435

Yeah. That comment is just in the wrong place.