[llvm] r347917 - [DAGCombiner] narrow truncated binops

Fri Dec 7 08:21:20 PST 2018

Sounds good. Thanks!

On Fri, Dec 7, 2018 at 5:08 PM Sanjay Patel <spatel at rotateright.com> wrote:
>
> I added a debug flag to disable this transform by default, so I can more easily investigate the bug:
> https://reviews.llvm.org/rL348604
>
>
> On Fri, Dec 7, 2018 at 8:08 AM Sanjay Patel <spatel at rotateright.com> wrote:
>>
>> 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