Page MenuHomePhabricator

Teach InlineCost about address spaces
AbandonedPublic

Authored by arsenm on Jul 31 2013, 1:08 PM.

Diff Detail

Event Timeline

chandlerc resigned from this revision.Mar 29 2015, 11:24 AM
chandlerc removed a reviewer: chandlerc.
bjope added a subscriber: bjope.Oct 11 2017, 1:40 AM

Does anyone remember why this has been "abandoned" (still not finished after four years being in need for review)?

I found some problem that seems to go away if we teach InlineCost about address spaces.

If we add the following test to test/Transforms/Inline/ptr-diff.ll then I see asserts like this:
opt: ../lib/IR/Constants.cpp:1834: static llvm::Constant *llvm::ConstantExpr::getCompare(unsigned short, llvm::Constant *, llvm::Constant *, bool): Assertion `C1->getType() == C2->getType() && "Op types should be identical!"' failed.
#0 0x0000000001d7f4c4 PrintStackTraceSignalHandler(void*) (../master/build-all/bin/opt+0x1d7f4c4)
#1 0x0000000001d7fc36 SignalHandler(int) (../master/build-all/bin/opt+0x1d7fc36)
#2 0x00007f7b0a120810 restore_rt (/lib64/libpthread.so.0+0xf810)
#3 0x00007f7b092cb875
GI_raise (/lib64/libc.so.6+0x32875)
#4 0x00007f7b092cce51 GI_abort (/lib64/libc.so.6+0x33e51)
#5 0x00007f7b092c4740
GI___assert_fail (/lib64/libc.so.6+0x2b740)
#6 0x00000000017d098a llvm::ConstantExpr::getCompare(unsigned short, llvm::Constant*, llvm::Constant*, bool) (../master/build-all/bin/opt+0x17d098a)
#7 0x000000000133bb8f (anonymous namespace)::CallAnalyzer::visitCmpInst(llvm::CmpInst&) (../master/build-all/bin/opt+0x133bb8f)
#8 0x000000000133662b llvm::InstVisitor<(anonymous namespace)::CallAnalyzer, bool>::visit(llvm::Instruction&) (../master/build-all/bin/opt+0x133662b)
#9 0x0000000001334fc4 (anonymous namespace)::CallAnalyzer::analyzeCall(llvm::CallSite) (../master/build-all/bin/opt+0x1334fc4)
#10 0x0000000001333848 llvm::getInlineCost(llvm::CallSite, llvm::Function*, llvm::InlineParams const&, llvm::TargetTransformInfo&, std::function<llvm::AssumptionCache& (llvm::Function&)>&, llvm::Optional<llvm::function_ref<llvm::BlockFrequencyInfo& (llvm::Function&)> >, llvm::ProfileSummaryInfo*, llvm::OptimizationRemarkEmitter*) (../master/build-all/bin/opt+0x1333848)
#11 0x000000000133345a llvm::getInlineCost(llvm::CallSite, llvm::InlineParams const&, llvm::TargetTransformInfo&, std::function<llvm::AssumptionCache& (llvm::Function&)>&, llvm::Optional<llvm::function_ref<llvm::BlockFrequencyInfo& (llvm::Function&)> >, llvm::ProfileSummaryInfo*, llvm::OptimizationRemarkEmitter*) (../master/build-all/bin/opt+0x133345a)
#12 0x0000000001939c3f (anonymous namespace)::SimpleInliner::getInlineCost(llvm::CallSite) (../master/build-all/bin/opt+0x1939c3f)
#13 0x000000000194065e shouldInline(llvm::CallSite, llvm::function_ref<llvm::InlineCost (llvm::CallSite)>, llvm::OptimizationRemarkEmitter&) (../master/build-all/bin/opt+0x194065e)
#14 0x000000000193af98 inlineCallsImpl(llvm::CallGraphSCC&, llvm::CallGraph&, std::function<llvm::AssumptionCache& (llvm::Function&)>, llvm::ProfileSummaryInfo*, llvm::TargetLibraryInfo&, bool, llvm::function_ref<llvm::InlineCost (llvm::CallSite)>, llvm::function_ref<llvm::AAResults& (llvm::Function&)>, llvm::ImportedFunctionsInliningStatistics&) (../master/build-all/bin/opt+0x193af98)
#15 0x000000000193a23a llvm::LegacyInlinerBase::inlineCalls(llvm::CallGraphSCC&) (../master/build-all/bin/opt+0x193a23a)
#16 0x000000000193a002 llvm::LegacyInlinerBase::runOnSCC(llvm::CallGraphSCC&) (../master/build-all/bin/opt+0x193a002)
#17 0x00000000012e330b (anonymous namespace)::CGPassManager::runOnModule(llvm::Module&) (../master/build-all/bin/opt+0x12e330b)
#18 0x000000000187f8e8 llvm::legacy::PassManagerImpl::run(llvm::Module&) (../master/build-all/bin/opt+0x187f8e8)
#19 0x000000000070411f main (../master/build-all/bin/opt+0x70411f)

Here is the test to add to test/Transforms/Inline/ptr-diff.ll:

;-------------------------------------------------------------
define i32 @outer3(i16* addrspace(1)* %ptr) {
; CHECK-LABEL: @outer3(
; CHECK-NOT: call i32
; CHECK: ret i32 3
; CHECK-LABEL: @inner3(
  %result = call i32 @inner3(i16* addrspace(1)* %ptr)
  ret i32 %result
}

define i32 @inner3(i16* addrspace(1)* %ptr) {
  call void @extern()
  %ptr.i = ptrtoint i16* addrspace(1)* %ptr to i64
  %distance = sub i64 %ptr.i, %ptr.i
  %icmp = icmp eq i64 %distance, 0
  br i1 %icmp, label %then, label %else

then:
  ret i32 3

else:
  ret i32 5
}
;-------------------------------------------------------------
bjope added a comment.Oct 16 2017, 2:19 PM

If nobody remembers why this very old patch never was landed, then I guess I'll make a new attempt as it would avoid the asserts that I've seen.
And it is also one step towards getting rid of the default arguments in DataLayout::getPointerSizeInBits(unsigned AS = 0) , marked as a FIXME in DataLayout.h.

bjope added a comment.Jan 4 2018, 10:33 AM

I believe this review is superseded by https://reviews.llvm.org/rL321809, so it can probably be abandoned now.

arsenm abandoned this revision.Jan 4 2018, 10:41 AM