Page MenuHomePhabricator

[PM] Schedule InstSimplify after late LICM run, to clean up LCSSA nodes.
ClosedPublic

Authored by mjacob on May 24 2016, 4:38 PM.

Details

Summary

The module pass pipeline includes a late LICM run after loop
unrolling. LCSSA is implicitly run as a pass dependency of LICM. However no
cleanup pass was run after this, so the LCSSA nodes ended in the optimized output.

Diff Detail

Event Timeline

mjacob updated this revision to Diff 58354.May 24 2016, 4:38 PM
mjacob retitled this revision from to [PM] Schedule InstCombine after late LICM run, to clean up LCSSA nodes..
mjacob updated this object.
mjacob added a reviewer: hfinkel.
mjacob added a subscriber: llvm-commits.
mjacob updated this object.May 24 2016, 4:38 PM
mehdi_amini added inline comments.May 24 2016, 4:49 PM
lib/Transforms/IPO/PassManagerBuilder.cpp
547

InstCombine is expensive, do we have an alternative? What is the impact of not doing this cleanup here?

mjacob updated this revision to Diff 58357.May 24 2016, 4:54 PM

Fix test.

mjacob added inline comments.May 24 2016, 5:09 PM
lib/Transforms/IPO/PassManagerBuilder.cpp
547

The LCSSA will end up in the optimized output. Two cases in which this was a problem for me:

  1. If I run additional passes after O3, e.g. late statepoint insertion for GC, they get feed with suboptimal code, possibly affecting their output. That can of course easily be worked around by running InstCombine manually in between.
  1. I'm working on a backend which directly emits code from LLVM IR. Coworkers complained about unnecessary phi nodes. Again this can be worked around by running InstCombine manually. However that possibly obfuscates the code when debugging.

I'm not sure whether this affects normal backends using SelectionDAG. At least I didn't find InstCombine in the llc pass pipeline.

For reference: here is the bug report I filed recently: https://llvm.org/bugs/show_bug.cgi?id=27620

lib/Transforms/IPO/PassManagerBuilder.cpp
547

Would InstSimplify work to get rid of these LCSSA PHI nodes?

mjacob updated this revision to Diff 58375.May 24 2016, 6:59 PM

Run much cheaper InstSimplify instead of InstCombine.

For reference: here is the bug report I filed recently: https://llvm.org/bugs/show_bug.cgi?id=27620

lib/Transforms/IPO/PassManagerBuilder.cpp
547

You're right. I was sure I tried this before.

mjacob retitled this revision from [PM] Schedule InstCombine after late LICM run, to clean up LCSSA nodes. to [PM] Schedule InstSimplify after late LICM run, to clean up LCSSA nodes..May 25 2016, 7:38 PM
mjacob added a comment.Jun 1 2016, 5:34 PM

*ping*

Should be uncontroversial now, with InstSimplify instead of InstCombine.

mehdi_amini accepted this revision.Jun 1 2016, 5:42 PM
mehdi_amini added a reviewer: mehdi_amini.
mehdi_amini added inline comments.
test/Other/cleanup-lcssa.ll
14

Are we running such tests usually?
i.e. running something generic as "O3" and asserting the presence or absence of some instructions?

This revision is now accepted and ready to land.Jun 1 2016, 5:42 PM
mjacob added inline comments.Jun 1 2016, 6:20 PM
test/Other/cleanup-lcssa.ll
14

I didn't find anything which was really comparable, but there are some tests that run opt -O3 .... test/Feature/OperandBundles/pr26510.ll even checks a whole function body.

mjacob closed this revision.Jun 2 2016, 3:20 PM