HomePhabricator

[Instruction] Speculatively undo isIdenticalToWhenDefined() PHI handling changes

Authored by lebedev.ri on Aug 29 2020, 9:38 AM.

Description

[Instruction] Speculatively undo isIdenticalToWhenDefined() PHI handling changes

The stage2-stage3 differences persist even without instcombine-based
PHI CSE, so this is the only possible reason.

Details

Committed
lebedev.riAug 29 2020, 9:38 AM
Parents
rG096527214033: [EarlyCSE] fold commutable intrinsics
Branches
Unknown
Tags
Unknown

Event Timeline

Hi Roman!
I don't know if this information will help you, but after c9dc627331992b01b962103fad813810cba00696 on internal testing we started hitting following assertion at very low rate

llvm/include/llvm/ADT/DenseMap.h:408 assert(!FoundVal && "Key already in new map?");

moveFromOldBuckets (OldBucketsEnd=0x7f297aa0cbf0, OldBucketsBegin=0x7f297aa0c9f0, this=0x7f2a388cc5c0)
    at llvm/include/llvm/ADT/DenseMap.h:408
llvm::DenseMap<llvm::PHINode*, llvm::detail::DenseSetEmpty, llvm::EliminateDuplicatePHINodes(llvm::BasicBlock*)::PHIDenseMapInfo, llvm::detail::DenseSetPair<llvm::PHINode*> >::grow(unsigned int) (
    this=this@entry=0x7f2a388cc5c0, AtLeast=<optimized out>) at llvm/include/llvm/ADT/DenseMap.h:813
grow (AtLeast=<optimized out>, this=<optimized out>) at llvm/include/llvm/ADT/DenseMap.h:537
InsertIntoBucketImpl<llvm::PHINode*> (Key=@0x7f2a388cc5b0: 0x7f29784b54c8, TheBucket=<optimized out>, Lookup=@0x7f2a388cc5b0: 0x7f29784b54c8, this=0x7f2a388cc5c0)
    at llvm/include/llvm/ADT/DenseMap.h:586
InsertIntoBucket<llvm::PHINode* const&, llvm::detail::DenseSetEmpty&> (Key=@0x7f2a388cc5b0: 0x7f29784b54c8, TheBucket=<optimized out>, this=0x7f2a388cc5c0)
    at llvm/include/llvm/ADT/DenseMap.h:547
try_emplace<llvm::detail::DenseSetEmpty&> (Key=@0x7f2a388cc5b0: 0x7f29784b54c8, this=0x7f2a388cc5c0)
    at llvm/include/llvm/ADT/DenseMap.h:258
insert (V=@0x7f2a388cc5b0: 0x7f29784b54c8, this=0x7f2a388cc5c0) at llvm/include/llvm/ADT/DenseSet.h:200
llvm::EliminateDuplicatePHINodes (BB=BB@entry=0x7f29790ec590) at llvm/lib/Transforms/Utils/Local.cpp:1172
simplifyOnce (BB=0x7f29790ec590, this=0x7f2a388cc690) at llvm/lib/Transforms/Utils/SimplifyCFG.cpp:6267
run (BB=<optimized out>, this=<optimized out>) at llvm/lib/Transforms/Utils/SimplifyCFG.cpp:6329
llvm::simplifyCFG (BB=<optimized out>, TTI=..., Options=..., LoopHeaders=LoopHeaders@entry=0x7f2a388cc870)
    at llvm/lib/Transforms/Utils/SimplifyCFG.cpp:6340
iterativelySimplifyCFG (F=..., TTI=..., Options=...) at llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp:184
simplifyFunctionCFG (F=..., TTI=..., Options=...) at llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp:198
llvm::SimplifyCFGPass::run (this=0x7f2a2bc832c8, F=..., AM=...)
    at llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp:244

And after this patch we no longer hit it.

Maybe it'll give you some insight on the implications of that change.
Unfortunately I can't provide an IR reproducer.

Hi Roman!

Hi.

I don't know if this information will help you, but after c9dc627331992b01b962103fad813810cba00696 on internal testing we started hitting following assertion at very low rate

llvm/include/llvm/ADT/DenseMap.h:408 assert(!FoundVal && "Key already in new map?");

moveFromOldBuckets (OldBucketsEnd=0x7f297aa0cbf0, OldBucketsBegin=0x7f297aa0c9f0, this=0x7f2a388cc5c0)
    at llvm/include/llvm/ADT/DenseMap.h:408
llvm::DenseMap<llvm::PHINode*, llvm::detail::DenseSetEmpty, llvm::EliminateDuplicatePHINodes(llvm::BasicBlock*)::PHIDenseMapInfo, llvm::detail::DenseSetPair<llvm::PHINode*> >::grow(unsigned int) (
    this=this@entry=0x7f2a388cc5c0, AtLeast=<optimized out>) at llvm/include/llvm/ADT/DenseMap.h:813
grow (AtLeast=<optimized out>, this=<optimized out>) at llvm/include/llvm/ADT/DenseMap.h:537
InsertIntoBucketImpl<llvm::PHINode*> (Key=@0x7f2a388cc5b0: 0x7f29784b54c8, TheBucket=<optimized out>, Lookup=@0x7f2a388cc5b0: 0x7f29784b54c8, this=0x7f2a388cc5c0)
    at llvm/include/llvm/ADT/DenseMap.h:586
InsertIntoBucket<llvm::PHINode* const&, llvm::detail::DenseSetEmpty&> (Key=@0x7f2a388cc5b0: 0x7f29784b54c8, TheBucket=<optimized out>, this=0x7f2a388cc5c0)
    at llvm/include/llvm/ADT/DenseMap.h:547
try_emplace<llvm::detail::DenseSetEmpty&> (Key=@0x7f2a388cc5b0: 0x7f29784b54c8, this=0x7f2a388cc5c0)
    at llvm/include/llvm/ADT/DenseMap.h:258
insert (V=@0x7f2a388cc5b0: 0x7f29784b54c8, this=0x7f2a388cc5c0) at llvm/include/llvm/ADT/DenseSet.h:200
llvm::EliminateDuplicatePHINodes (BB=BB@entry=0x7f29790ec590) at llvm/lib/Transforms/Utils/Local.cpp:1172
simplifyOnce (BB=0x7f29790ec590, this=0x7f2a388cc690) at llvm/lib/Transforms/Utils/SimplifyCFG.cpp:6267
run (BB=<optimized out>, this=<optimized out>) at llvm/lib/Transforms/Utils/SimplifyCFG.cpp:6329
llvm::simplifyCFG (BB=<optimized out>, TTI=..., Options=..., LoopHeaders=LoopHeaders@entry=0x7f2a388cc870)
    at llvm/lib/Transforms/Utils/SimplifyCFG.cpp:6340
iterativelySimplifyCFG (F=..., TTI=..., Options=...) at llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp:184
simplifyFunctionCFG (F=..., TTI=..., Options=...) at llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp:198
llvm::SimplifyCFGPass::run (this=0x7f2a2bc832c8, F=..., AM=...)
    at llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp:244

And after this patch we no longer hit it.

Maybe it'll give you some insight on the implications of that change.
Unfortunately I can't provide an IR reproducer.

Yeah, we've since then established that we can't just change instruction identity check,
but also have to change the instruction hashing.
For EarlyCSE -earlycse-debug-hash verifies that invariant.
For that crash you were seeing, which was yet another manifestation of the miscompile causing stage2-stage3 mismatch,
i've already added rG961483a5ea7c0e628c025187287d1658457ffcb3 that would have caught it/this.