Page MenuHomePhabricator

Explicitly require DominatorTreeAnalysis pass for instsimplify pass.
ClosedPublic

Authored by danielcdh on Sep 1 2016, 4:58 PM.

Diff Detail

Event Timeline

danielcdh updated this revision to Diff 70103.Sep 1 2016, 4:58 PM
danielcdh retitled this revision from to Explicitly require DominatorTreeAnalysis pass for instsimplify pass..
danielcdh updated this object.
danielcdh added a reviewer: davidxl.
danielcdh added a subscriber: llvm-commits.
danielcdh accepted this revision.Sep 1 2016, 4:59 PM
danielcdh added a reviewer: danielcdh.

commit without review.

This revision is now accepted and ready to land.Sep 1 2016, 4:59 PM
danielcdh closed this revision.Sep 1 2016, 4:59 PM
danielcdh reopened this revision.Sep 1 2016, 7:02 PM

saw a bunch of buildbot failure pointing to this one, thus revert the change, investigating...

This revision is now accepted and ready to land.Sep 1 2016, 7:02 PM

Looks like the buildbot failure are due to the other patch. But I'll hold off until I get an approval before I commit the patch again.

Change #123918

Changed by alexshap
Changed at Thu 01 Sep 2016 17:00:30
Repository http://llvm.org/svn/llvm-project
Project clang-tools-extra
Branch trunk
Revision 280431
Comments

Add clang-reorder-fields to clang-tools-extra

This diff adds v0 of clang-reorder-fields tool to clang/tools/extra.
The main idea behind this tool is to simplify and make less error-prone refactoring of large codebases when
someone needs to change the order fields of a struct/class (for example to remove excess padding).

Differential revision: https://reviews.llvm.org/D23279
Changed files

CMakeLists.txt
clang-reorder-fields
clang-reorder-fields/CMakeLists.txt
clang-reorder-fields/ReorderFieldsAction.cpp
clang-reorder-fields/ReorderFieldsAction.h
clang-reorder-fields/tool
clang-reorder-fields/tool/CMakeLists.txt
clang-reorder-fields/tool/ClangReorderFields.cpp
test/CMakeLists.txt
test/clang-reorder-fields
test/clang-reorder-fields/AggregatePartialInitialization.cpp
test/clang-reorder-fields/CStructAmbiguousName.cpp
test/clang-reorder-fields/CStructFieldsOrder.cpp
test/clang-reorder-fields/ClassDifferentFieldsAccesses.cpp
test/clang-reorder-fields/ClassMixedInitialization.cpp
test/clang-reorder-fields/ClassSimpleCtor.cpp

davidxl edited edge metadata.Sep 6 2016, 2:42 PM

What breaks without the change?

What breaks without the change?

When I enable LoopSink pass, I need to add InstSimplifierPass pass after it to do some cleanup. However, if I directly invoke InstSimplifierPass pass, it will fail due to lack of DT.

From the InstSimplifierPass implementation, it uses DT without checking if it is nullptr. So I think we should explicitly require DT for this pass.

A typical use of DT from InstSimplifierPass is:

llvm::SimplifyInstruction->
llvm::SimplifyICmpInst->
SimplifyICmpInst ->
computePointerICmp->
llvm::isKnownNonNullAt (ValueTracking.cpp) ->
isKnownNonNullFromDominatingCondition (ValueTracking.cpp) ->
llvm::DominatorTree::dominates (Dominators.cpp)

davidxl accepted this revision.Sep 6 2016, 3:22 PM
davidxl edited edge metadata.

lgtm

danielcdh closed this revision.Sep 6 2016, 3:25 PM

The change seems fine, but you should add a test that demonstrates the failure. Any IR input that triggers the code path you suggest run with just the inst simplify pass from opt should suffice?

Test added in r280963.