HomePhabricator

Recommit r333268: [IPSCCP] Use PredicateInfo to propagate facts from cmp…
Concern RaisedrL333740

Description

Recommit r333268: [IPSCCP] Use PredicateInfo to propagate facts from cmp instructions.

This patch updates IPSCCP to use PredicateInfo to propagate
facts to true branches predicated by EQ and to false branches
predicated by NE.

As a follow up, we should be able to extend it to also propagate additional
facts about nonnull.

Reviewers: davide, mssimpso, dberlin, efriedma

Reviewed By: davide, dberlin

Differential Revision: https://reviews.llvm.org/D45330

Details

Auditors
thegameg
Committed
fhahnJun 1 2018, 3:48 AM
Reviewer
davide
Differential Revision
D45330: [IPSCCP] Use PredicateInfo to propagate facts from cmp instructions.
Parents
rL333739: [mips] Guard 'nop' properly and add mips16's nop instruction
Branches
Unknown
Tags
Unknown

Event Timeline

thegameg raised a concern with this commit.Jun 21 2018, 8:46 AM
thegameg added a subscriber: thegameg.

Hi @fhahn, this commit seems to break some internal tests of ours. I managed to reduce the assert to the following test:

; RUN: opt -ipsccp -o /dev/null -mtriple=aarch64--
; RUN: opt -ipsccp -o /dev/null -mtriple=x86_64--

%0 = type opaque
%1 = type opaque

declare i8* @fun(%1*)

define void @f0(%0* %arg) {
bb:
  %tmp = icmp ne %0* %arg, null
  br i1 %tmp, label %bb1, label %bb3

bb1:                                              ; preds = %bb
  %tmp2 = bitcast %0* %arg to i8*
  unreachable

bb3:                                              ; preds = %bb
  ret void
}

define void @f1(%1* %tmp) {
bb:
  %tmp1 = icmp eq %1* null, %tmp
  br i1 %tmp1, label %bb2, label %bb3

bb2:                                              ; preds = %bb
  ret void

bb3:                                              ; preds = %bb
  call i8* @fun(%1* %tmp)
  ret void
}

This asserts:

Assertion failed: (isa<X>(Val) && "cast<Ty>() argument of incompatible type!"), function cast, file ../include/llvm/Support/Casting.h, line 255.
Stack dump:
0.	Program arguments: ./build/bin/opt -ipsccp reduced.ll -mtriple=aarch64-- -o /dev/null
1.	Running pass 'Interprocedural Sparse Conditional Constant Propagation' on module 'reduced.ll'.
0  opt                      0x000000010481994c llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 60
1  opt                      0x0000000104819f19 PrintStackTraceSignalHandler(void*) + 25
2  opt                      0x000000010481670e llvm::sys::RunSignalHandlers() + 990
3  opt                      0x000000010481ab25 SignalHandler(int) + 485
4  libsystem_platform.dylib 0x00007fff74dcbd3a _sigtramp + 26
5  libsystem_platform.dylib 0x0000000000015520 _sigtramp + 2334431232
6  libsystem_c.dylib        0x00007fff74c8d09d abort + 127
7  libsystem_c.dylib        0x00007fff74c5578e __assert_rtn + 320
8  opt                      0x000000010398d847 llvm::cast_retty<llvm::Function, llvm::Constant*>::ret_type llvm::cast<llvm::Function, llvm::Constant>(llvm::Constant*) + 103
9  opt                      0x0000000103ad50d6 llvm::Intrinsic::getDeclaration(llvm::Module*, llvm::Intrinsic::ID, llvm::ArrayRef<llvm::Type*>) + 742
10 opt                      0x000000010491e192 llvm::PredicateInfo::materializeStack(unsigned int&, llvm::SmallVectorImpl<llvm::PredicateInfoClasses::ValueDFS>&, llvm::Value*) + 850
11 opt                      0x000000010491dc18 llvm::PredicateInfo::renameUses(llvm::SmallPtrSetImpl<llvm::Value*>&) + 3480
12 opt                      0x000000010491ce0c llvm::PredicateInfo::buildPredicateInfo() + 780
13 opt                      0x000000010491efbc llvm::PredicateInfo::PredicateInfo(llvm::Function&, llvm::DominatorTree&, llvm::AssumptionCache&) + 188
14 opt                      0x000000010491f06d llvm::PredicateInfo::PredicateInfo(llvm::Function&, llvm::DominatorTree&, llvm::AssumptionCache&) + 45
15 opt                      0x0000000103e5d588 std::__1::enable_if<!(std::is_array<llvm::PredicateInfo>::value), std::__1::unique_ptr<llvm::PredicateInfo, std::__1::default_delete<llvm::PredicateInfo> > >::type llvm::make_unique<llvm::PredicateInfo, llvm::Function&, llvm::DominatorTree&, llvm::AssumptionCache&>(llvm::Function&&&, llvm::DominatorTree&&&, llvm::AssumptionCache&&&) + 120
16 opt                      0x0000000103e5d4f8 (anonymous namespace)::IPSCCPLegacyPass::runOnModule(llvm::Module&)::'lambda'(llvm::Function&)::operator()(llvm::Function&) const + 120
17 opt                      0x0000000103e5d46c std::__1::unique_ptr<llvm::PredicateInfo, std::__1::default_delete<llvm::PredicateInfo> > llvm::function_ref<std::__1::unique_ptr<llvm::PredicateInfo, std::__1::default_delete<llvm::PredicateInfo> > (llvm::Function&)>::callback_fn<(anonymous namespace)::IPSCCPLegacyPass::runOnModule(llvm::Module&)::'lambda'(llvm::Function&)>(long, llvm::Function&) + 44
18 opt                      0x000000010455bfbe llvm::function_ref<std::__1::unique_ptr<llvm::PredicateInfo, std::__1::default_delete<llvm::PredicateInfo> > (llvm::Function&)>::operator()(llvm::Function&) const + 62
19 opt                      0x000000010455a80f llvm::runIPSCCP(llvm::Module&, llvm::DataLayout const&, llvm::TargetLibraryInfo const*, llvm::function_ref<std::__1::unique_ptr<llvm::PredicateInfo, std::__1::default_delete<llvm::PredicateInfo> > (llvm::Function&)>) + 271
20 opt                      0x0000000103e5d3ac (anonymous namespace)::IPSCCPLegacyPass::runOnModule(llvm::Module&) + 172
21 opt                      0x0000000103b52b50 (anonymous namespace)::MPPassManager::runOnModule(llvm::Module&) + 1504
22 opt                      0x0000000103b522f6 llvm::legacy::PassManagerImpl::run(llvm::Module&) + 342
23 opt                      0x0000000103b535c1 llvm::legacy::PassManager::run(llvm::Module&) + 33
24 opt                      0x00000001023bd644 main + 27796
25 libdyld.dylib            0x00007fff74bd1b11 start + 1

Could you please take a look?

This commit now has outstanding concerns.Jun 21 2018, 8:46 AM
fhahn added a comment.Jun 21 2018, 9:11 AM

Interesting, thanks for raising that! Looks like an issue with PredicateInfo (which is just used by this change). I'll prepare a fix.

Interesting, thanks for raising that! Looks like an issue with PredicateInfo (which is just used by this change). I'll prepare a fix.

Great! Thanks!

xbolva00 added inline comments.
/llvm/trunk/lib/Transforms/Scalar/SCCP.cpp
261

Nit: I->getFunction()?

Ok so the problem seems to be that there are two different types %0 = type opaque , %1 = type opaque and both get mangled to the same string for the intrinsic declaration. So Intrinsic::getDeclaration('ssa.copy', %0) returns something like llvm.ssa.copy.p0s_s(%0 returned) . Then for type %1, we get the existing declaration (with the same name), but it has a different paramter type (%0) and we need a bitcast. Intrinsic::getDeclaration does not expect that (it casts it to Function, which fails) and there is a comment, stating that there should never be multiple globals with the same name but different types.... Maybe a something is missing here with respect to the mangling of the types?

I have to run now, and will only be able to have a look tomorrow. If it's a big issue for you, would you mind reverting the patch?

/llvm/trunk/lib/Transforms/Scalar/SCCP.cpp
261

Will do, thanks

I have to run now, and will only be able to have a look tomorrow. If it's a big issue for you, would you mind reverting the patch?

Reverted in r335272.

fhahn added a comment.Jun 25 2018, 4:02 AM

Thanks! I've put up D48541 to address the mangling problem.

fhahn added a comment.Jul 25 2018, 8:02 AM

I've recommitted this as rL337904. A fix for the unnamed type issue went in earlier this week. Please let me know if there are any additional problems.