Page MenuHomePhabricator

[SimplifyCFG] mergeConditionalStoreToAddress(): consider cost, not instruction count
ClosedPublic

Authored by lebedev.ri on Sep 7 2019, 3:57 AM.

Details

Summary

As it can be see in the changed test, while div is really costly,
we were speculating it. This does not seem correct.

Also, the old code would run for every single insturuction in BB,
instead of eagerly bailing out as soon as there are too many instructions.

This function still has a problem that PHINodeFoldingThreshold is
per-basic-block, while it should be for all the basic blocks.

Diff Detail

Repository
rL LLVM

Event Timeline

lebedev.ri created this revision.Sep 7 2019, 3:57 AM

Any stats for this patch from your rawspeed?

Any stats for this patch from your rawspeed?

Didn't look; this patch only exists because of @efriedma's D65148#1661691 + D65148#1662019

jmolloy accepted this revision.Sep 18 2019, 11:00 AM
This revision is now accepted and ready to land.Sep 18 2019, 11:00 AM

On Wed, Sep 18, 2019 at 9:02 PM James Molloy via Phabricator <reviews@reviews.llvm.org> wrote:

jmolloy accepted this revision.
This revision is now accepted and ready to land.

Note: i don't know that this is the correct thing to do.
The old "cost" calculation seems, wrong?

This revision was automatically updated to reflect the committed changes.

When running test/Transforms/SimplifyCFG/merge-cond-stores-2.ll.test under address sanitizer, this revision creates a stack-use-after-scope failure at llvm/lib/Transforms/Utils/SimplifyCFG.cpp:3008:13:

==2292==ERROR: AddressSanitizer: stack-use-after-scope on address 0x7f8214d0a180 at pc 0x5609bb711de8 bp 0x7ffe8bc79080 sp 0x7ffe8bc79078
READ of size 8 at 0x7f8214d0a180 thread T0
    #0 0x5609bb711de7 in llvm::StoreInst* const* std::__u::find<llvm::StoreInst* const*, llvm::StoreInst*>(llvm::StoreInst* const*, llvm::StoreInst* const*, llvm::StoreInst* const&) include/c++/v1/algorithm:919:13
    #1 0x5609bb6fc7a1 in mergeConditionalStoreToAddress(llvm::BasicBlock*, llvm::BasicBlock*, llvm::BasicBlock*, llvm::BasicBlock*, llvm::BasicBlock*, llvm::Value*, bool, bool, llvm::DataLayout const&, llvm::TargetTransformInfo const&)::$_12::operator()(llvm::BasicBlock*, llvm::ArrayRef<llvm::StoreInst*>) const lib/Transforms/Utils/SimplifyCFG.cpp:3008:13
    #2 0x5609bb6fb8c3 in mergeConditionalStoreToAddress(llvm::BasicBlock*, llvm::BasicBlock*, llvm::BasicBlock*, llvm::BasicBlock*, llvm::BasicBlock*, llvm::Value*, bool, bool, llvm::DataLayout const&, llvm::TargetTransformInfo const&) lib/Transforms/Utils/SimplifyCFG.cpp:3027:43
    #3 0x5609bb6f2e84 in mergeConditionalStores(llvm::BranchInst*, llvm::BranchInst*, llvm::DataLayout const&, llvm::TargetTransformInfo const&) lib/Transforms/Utils/SimplifyCFG.cpp:3210:16
    #4 0x5609bb6f0e1c in SimplifyCondBranchToCondBranch(llvm::BranchInst*, llvm::BranchInst*, llvm::DataLayout const&, llvm::TargetTransformInfo const&) lib/Transforms/Utils/SimplifyCFG.cpp:3277:26
    #5 0x5609bb6e2db2 in (anonymous namespace)::SimplifyCFGOpt::SimplifyCondBranch(llvm::BranchInst*, llvm::IRBuilder<llvm::ConstantFolder, llvm::IRBuilderDefaultInserter>&) lib/Transforms/Utils/SimplifyCFG.cpp:5949:13
    #6 0x5609bb6e0122 in (anonymous namespace)::SimplifyCFGOpt::simplifyOnce(llvm::BasicBlock*) lib/Transforms/Utils/SimplifyCFG.cpp:6089:11
    #7 0x5609bb6dfc9c in (anonymous namespace)::SimplifyCFGOpt::run(llvm::BasicBlock*) lib/Transforms/Utils/SimplifyCFG.cpp:6124:16
    #8 0x5609bb6dfb6a in llvm::simplifyCFG(llvm::BasicBlock*, llvm::TargetTransformInfo const&, llvm::SimplifyCFGOptions const&, llvm::SmallPtrSetImpl<llvm::BasicBlock*>*) lib/Transforms/Utils/SimplifyCFG.cpp:6135:8
    #9 0x5609bb2167b1 in iterativelySimplifyCFG(llvm::Function&, llvm::TargetTransformInfo const&, llvm::SimplifyCFGOptions const&) lib/Transforms/Scalar/SimplifyCFGPass.cpp:163:11
    #10 0x5609bb2156dc in simplifyFunctionCFG(llvm::Function&, llvm::TargetTransformInfo const&, llvm::SimplifyCFGOptions const&) lib/Transforms/Scalar/SimplifyCFGPass.cpp:177:18
    #11 0x5609bc3833b9 in llvm::FPPassManager::runOnFunction(llvm::Function&) lib/IR/LegacyPassManager.cpp:1648:27
    #12 0x5609bc3837c2 in llvm::FPPassManager::runOnModule(llvm::Module&) lib/IR/LegacyPassManager.cpp:1685:16
    #13 0x5609bc384138 in (anonymous namespace)::MPPassManager::runOnModule(llvm::Module&) lib/IR/LegacyPassManager.cpp:1750:27
...

When running test/Transforms/SimplifyCFG/merge-cond-stores-2.ll.test under address sanitizer, this revision creates a stack-use-after-scope failure at llvm/lib/Transforms/Utils/SimplifyCFG.cpp:3008:13:

Does this still happen after rL372262?

==2292==ERROR: AddressSanitizer: stack-use-after-scope on address 0x7f8214d0a180 at pc 0x5609bb711de8 bp 0x7ffe8bc79080 sp 0x7ffe8bc79078
READ of size 8 at 0x7f8214d0a180 thread T0
    #0 0x5609bb711de7 in llvm::StoreInst* const* std::__u::find<llvm::StoreInst* const*, llvm::StoreInst*>(llvm::StoreInst* const*, llvm::StoreInst* const*, llvm::StoreInst* const&) include/c++/v1/algorithm:919:13
    #1 0x5609bb6fc7a1 in mergeConditionalStoreToAddress(llvm::BasicBlock*, llvm::BasicBlock*, llvm::BasicBlock*, llvm::BasicBlock*, llvm::BasicBlock*, llvm::Value*, bool, bool, llvm::DataLayout const&, llvm::TargetTransformInfo const&)::$_12::operator()(llvm::BasicBlock*, llvm::ArrayRef<llvm::StoreInst*>) const lib/Transforms/Utils/SimplifyCFG.cpp:3008:13
    #2 0x5609bb6fb8c3 in mergeConditionalStoreToAddress(llvm::BasicBlock*, llvm::BasicBlock*, llvm::BasicBlock*, llvm::BasicBlock*, llvm::BasicBlock*, llvm::Value*, bool, bool, llvm::DataLayout const&, llvm::TargetTransformInfo const&) lib/Transforms/Utils/SimplifyCFG.cpp:3027:43
    #3 0x5609bb6f2e84 in mergeConditionalStores(llvm::BranchInst*, llvm::BranchInst*, llvm::DataLayout const&, llvm::TargetTransformInfo const&) lib/Transforms/Utils/SimplifyCFG.cpp:3210:16
    #4 0x5609bb6f0e1c in SimplifyCondBranchToCondBranch(llvm::BranchInst*, llvm::BranchInst*, llvm::DataLayout const&, llvm::TargetTransformInfo const&) lib/Transforms/Utils/SimplifyCFG.cpp:3277:26
    #5 0x5609bb6e2db2 in (anonymous namespace)::SimplifyCFGOpt::SimplifyCondBranch(llvm::BranchInst*, llvm::IRBuilder<llvm::ConstantFolder, llvm::IRBuilderDefaultInserter>&) lib/Transforms/Utils/SimplifyCFG.cpp:5949:13
    #6 0x5609bb6e0122 in (anonymous namespace)::SimplifyCFGOpt::simplifyOnce(llvm::BasicBlock*) lib/Transforms/Utils/SimplifyCFG.cpp:6089:11
    #7 0x5609bb6dfc9c in (anonymous namespace)::SimplifyCFGOpt::run(llvm::BasicBlock*) lib/Transforms/Utils/SimplifyCFG.cpp:6124:16
    #8 0x5609bb6dfb6a in llvm::simplifyCFG(llvm::BasicBlock*, llvm::TargetTransformInfo const&, llvm::SimplifyCFGOptions const&, llvm::SmallPtrSetImpl<llvm::BasicBlock*>*) lib/Transforms/Utils/SimplifyCFG.cpp:6135:8
    #9 0x5609bb2167b1 in iterativelySimplifyCFG(llvm::Function&, llvm::TargetTransformInfo const&, llvm::SimplifyCFGOptions const&) lib/Transforms/Scalar/SimplifyCFGPass.cpp:163:11
    #10 0x5609bb2156dc in simplifyFunctionCFG(llvm::Function&, llvm::TargetTransformInfo const&, llvm::SimplifyCFGOptions const&) lib/Transforms/Scalar/SimplifyCFGPass.cpp:177:18
    #11 0x5609bc3833b9 in llvm::FPPassManager::runOnFunction(llvm::Function&) lib/IR/LegacyPassManager.cpp:1648:27
    #12 0x5609bc3837c2 in llvm::FPPassManager::runOnModule(llvm::Module&) lib/IR/LegacyPassManager.cpp:1685:16
    #13 0x5609bc384138 in (anonymous namespace)::MPPassManager::runOnModule(llvm::Module&) lib/IR/LegacyPassManager.cpp:1750:27
...