[llvm] r347917 - [DAGCombiner] narrow truncated binops

Fri Dec 7 07:08:59 PST 2018

Thanks. Yes, I agree - now that we have a repro test and know it's not
limited to PPC, we should revert. I'll do that shortly.

On Fri, Dec 7, 2018 at 5:30 AM Hans Wennborg via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> I just bisected an x86 Android Chrome build failure to this revision,
> so it's not PPC specific.
>
> I've uploaded a repro here:
> https://bugs.chromium.org/p/chromium/issues/detail?id=912878#c3
>
> Before this change, the test compiles in about 20 s on my machine.
> After the change, it runs for several minutes and then seems to hit an
> assert in SmallVector (due to out-of-memory I suppose).
>
> Since we now have a non-PPC test case that clearly shows this is a
> regression, I think the change should be reverted until it's been
> investigated.
>
> On Fri, Dec 7, 2018 at 2:01 AM Nemanja Ivanovic via llvm-commits
> <llvm-commits at lists.llvm.org> wrote:
> >
> > I am sorry that I am jumping into this so late and I don't know how it
> is that I am missing some of the original context. At some point there were
> mentions of a PPC specific problem. However this doesn't seem to be the
> case at this point. If the group on this thread still feels that there is a
> PPC specific component to this, please let me know how to reproduce and I
> will gladly do the PPC investigation.
> >
> > On Dec 6, 2018 7:26 PM, "Craig Topper" <craig.topper at gmail.com> wrote:
> >
> > DAGCombiner::Run makes sure all nodes are visited at least once. It does
> this by checking the operands for everything that's visited. So if the node
> that's being added to the worklist was created just before the add to
> worklist then I don't think it needs to be explicitly added.
> >
> > ~Craig
> >
> >
> > On Thu, Dec 6, 2018 at 4:13 PM Sanjay Patel <spatel at rotateright.com>
> wrote:
> >>
> >> Ok...there are still some seemingly obvious bugs in
> hoistLogicOpWithSameOpcodeHands(), so I'm going to keep trying to kill
> those.
> >>
> >> But I've hit a couple of things I don't understand:
> >> 1. Does that code need to explicitly call AddToWorklist()?
> >> 2. How did this happen?
> >> https://reviews.llvm.org/rL348552
> >>
> >> On Thu, Dec 6, 2018 at 3:52 PM David Jones <dlj at google.com> wrote:
> >>>
> >>> Sadly, I don't think r348508 fixes the issue. I still see the large
> memory growth. I collected some more traces from the allocator, and it
> looks like there is still quite a bit of allocation happening in
> hoistLogicOpWithSameOpcodeHands, but most of the growth is actually coming
> from further up the stack (SelectionDAG::Combine).
> >>>
> >>> On Thu, Dec 6, 2018 at 10:27 AM Sanjay Patel <spatel at rotateright.com>
> wrote:
> >>>>
> >>>> First attempt at damage control in that function:
> >>>> https://reviews.llvm.org/rL348508
> >>>>
> >>>> If I've guessed correctly, that's enough to prevent the OOM. Still
> looking at fixing that better though.
> >>>>
> >>>>
> >>>> On Thu, Dec 6, 2018 at 8:12 AM Sanjay Patel <spatel at rotateright.com>
> wrote:
> >>>>>
> >>>>> #10 0x000055680f8d7466 (anonymous
> namespace)::DAGCombiner::SimplifyBinOpWithSameOpcodeHands(llvm::SDNode*)
> >>>>>
> >>>>> Thanks! That's a good lead. I'm looking at that code now, and it
> doesn't appear to check the number of uses before doing transforms. That
> means it can increase the instruction count at each invocation. Let me try
> some experiments in there.
> >>>>>
> >>>>> On Wed, Dec 5, 2018 at 7:51 PM David Jones <dlj at google.com> wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On Wed, Dec 5, 2018 at 6:34 PM Friedman, Eli <
> efriedma at codeaurora.org> wrote:
> >>>>>>>
> >>>>>>> On 12/5/2018 5:47 PM, David Jones wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>> On Wed, Dec 5, 2018 at 5:40 PM Sanjay Patel <
> spatel at rotateright.com> wrote:
> >>>>>>>>
> >>>>>>>> Hi David,
> >>>>>>>>
> >>>>>>>> Thanks for reporting the problem. I don’t have any guesses as to
> how this could cause OOM. Cc’ing some people that might.
> >>>>>>>
> >>>>>>>
> >>>>>>> +a few more, thanks! :-)
> >>>>>>>
> >>>>>>>>
> >>>>>>>> If the problem is only showing up on PPC, a quick hack would be
> to add a TLI hook and disable the transform for PPC. I’d prefer that to a
> full revert (especially since I’ve already enhanced this code to handle
> some vectors, and another enhancement is in progress)...unless we have a
> test case that shows it’s not a PPC-specific bug?
> >>>>>>>>
> >>>>>>>
> >>>>>>> Agreed on avoiding a revert... this looks like a pretty
> hard-fought change, based on https://bugs.llvm.org/show_bug.cgi?id=32023.
> >>>>>>>
> >>>>>>> I do believe this is PPC-specific (although, as is often the case,
> your change may simply be tickling a bug somewhere else).
> >>>>>>>
> >>>>>>> If it's PPC-specific, my first guess here would be there's some
> PowerPC-specific target combine that's trying to combine the opposite way,
> or something like that.  Assuming that's the case, it probably runs out of
> memory pretty quickly, so it should be fast to bisect if you restrict the
> memory usage with ulimit.
> >>>>>>
> >>>>>>
> >>>>>> So I did try something like this... instead of just ulimit, I
> patched my allocator to crash when it sees a sufficiently large allocation
> (so I can see the large allocation itself, not just a random allocation
> when close to the limit).
> >>>>>>
> >>>>>> Here's a stacktrace I got (slightly edited for clarity):
> >>>>>>
> >>>>>> 1. <eof> parser at end of file
> >>>>>> 2. Code generation
> >>>>>> 3. Running pass 'Function Pass Manager' on module
> 'FrontendAction-9a7810.cpp'.
> >>>>>> 4. Running pass 'PowerPC DAG->DAG Pattern Instruction Selection' on
> function
> '@_ZN5clang14FrontendAction15BeginSourceFileERNS_16CompilerInstanceERKNS_17FrontendInputFileE'
> >>>>>> #0 0x000055681055d388 llvm::sys::RunSignalHandlers()
> >>>>>> #1 0x000055681055f486 SignalHandler(int)
> >>>>>> #2 0x00007f88bd9f89a0 __restore_rt
> >>>>>> #3 0x00007f88bd77a602 __GI_raise
> >>>>>> #4 0x00007f88bd77c320 __GI_abort
> >>>>>> #5 0x00005568105e2a70 tcmalloc::Logger::Add(tcmalloc::LogItem
> const&)
> >>>>>> #6 0x00005568105d5838 (anonymous
> namespace)::do_malloc_pages(unsigned long, unsigned long)
> >>>>>> #7 0x000055681068a56d realloc
> >>>>>> #8 0x000055681055c789 llvm::SmallVectorBase::grow_pod(void*,
> unsigned long, unsigned long)
> >>>>>> #9 0x000055680f895119 (anonymous
> namespace)::DAGCombiner::AddToWorklist(llvm::SDNode*)
> >>>>>> #10 0x000055680f8d7466 (anonymous
> namespace)::DAGCombiner::SimplifyBinOpWithSameOpcodeHands(llvm::SDNode*)
> >>>>>> #11 0x000055680f8a1791 (anonymous
> namespace)::DAGCombiner::visitAND(llvm::SDNode*)
> >>>>>> #12 0x000055680f89729b (anonymous
> namespace)::DAGCombiner::combine(llvm::SDNode*)
> >>>>>> #13 0x000055680f8960cf
> llvm::SelectionDAG::Combine(llvm::CombineLevel, llvm::AAResults*,
> llvm::CodeGenOpt::Level)
> >>>>>> #14 0x000055680f78cdbc llvm::SelectionDAGISel::CodeGenAndEmitDAG()
> >>>>>> #15 0x000055680f78bbde
> llvm::SelectionDAGISel::SelectAllBasicBlocks(llvm::Function const&)
> >>>>>> #16 0x000055680f788c56
> llvm::SelectionDAGISel::runOnMachineFunction(llvm::MachineFunction&)
> >>>>>> #17 0x000055680f418574 (anonymous
> namespace)::PPCDAGToDAGISel::runOnMachineFunction(llvm::MachineFunction&)
> >>>>>> #18 0x000055680fa0955b
> llvm::MachineFunctionPass::runOnFunction(llvm::Function&)
> >>>>>> #19 0x0000556810478c6c
> llvm::FPPassManager::runOnFunction(llvm::Function&)
> >>>>>> #20 0x0000556810478f23
> llvm::FPPassManager::runOnModule(llvm::Module&)
> >>>>>> #21 0x00005568104793f5
> llvm::legacy::PassManagerImpl::run(llvm::Module&)
> >>>>>> #22 0x000055680da6c9ff
> clang::EmitBackendOutput(clang::DiagnosticsEngine&,
> clang::HeaderSearchOptions const&, clang::CodeGenOptions const&,
> clang::TargetOptions const&, clang::LangOptions const&, llvm::DataLayout
> const&, llvm::Module*, clang::BackendAction,
> std::unique_ptr<llvm::raw_pwrite_stream,
> std::default_delete<llvm::raw_pwrite_stream> >)
> >>>>>> #23 0x000055680da43542
> clang::BackendConsumer::HandleTranslationUnit(clang::ASTContext&)
> >>>>>> #24 0x000055680e47c7b3 clang::ParseAST(clang::Sema&, bool, bool)
> >>>>>> #25 0x000055680e2bc0fa clang::FrontendAction::Execute()
> >>>>>> #26 0x000055680e142428
> clang::CompilerInstance::ExecuteAction(clang::FrontendAction&)
> >>>>>> #27 0x000055680d80b562
> clang::ExecuteCompilerInvocation(clang::CompilerInstance*)
> >>>>>> #28 0x000055680d7fab05 cc1_main(llvm::ArrayRef<char const*>, char
> const*, void*)
> >>>>>> #29 0x000055680d8088ea main
> >>>>>> #30 0x00007f88bd766bbd __libc_start_main
> >>>>>> #31 0x000055680d7fa469 _start
> >>>>>>
> >>>>>>
> >>>>>> (I've been poking at this in a few other ways... the stacks vary
> somewhat, but they all seem to point to an ever-growing work list in
> DAGCombiner, inside of PPCDAGToDAGISel -- so #13-#31 seem to be the common
> parts of the stacks.)
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20181207/4c9c3979/attachment.html>