[llvm] r278532 - ADT: Remove the ilist_nextprev_traits customization point
Duncan P. N. Exon Smith via llvm-commits
llvm-commits at lists.llvm.org
Fri Aug 12 17:13:11 PDT 2016
> On 2016-Aug-12, at 16:45, Hans Wennborg <hans at chromium.org> wrote:
>
> On Fri, Aug 12, 2016 at 4:15 PM, Duncan P. N. Exon Smith
> <dexonsmith at apple.com> wrote:
>>
>>> On 2016-Aug-12, at 16:12, Yung, Douglas <douglas.yung at sony.com> wrote:
>>>
>>> Hi Duncan,
>>>
>>> Your change in r278572 seems to have helped, the number of failures is down to 8. (http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/9911).
>>
>> Yes, I had just seen that myself and was about to comment!
>>
>>> (The sancov failure is unrelated to your change) I'm still building a debug build of the compiler and will send a callstack for the failure when I have it.
>>
>> Thanks very much. Let me know if you need me to revert r278532-42 while we investigate (happy to revert as long as you can send me the backtraces!).
>
> I committed r278577 for another instance of this. Halfway through a
> Chromium build now :-)
Great! Thanks Hans.
>
>
>>>> -----Original Message-----
>>>> From: dexonsmith at apple.com [mailto:dexonsmith at apple.com]
>>>> Sent: Friday, August 12, 2016 16:05
>>>> To: Yung, Douglas
>>>> Cc: Hans Wennborg; llvm-commits at lists.llvm.org
>>>> Subject: Re: [llvm] r278532 - ADT: Remove the ilist_nextprev_traits
>>>> customization point
>>>>
>>>> Can you give me the backtraces? I strongly suspect a similar problem,
>>>> but I can't investigate on Darwin without knowing where it crashes.
>>>> (Perhaps r278572 will be sufficient...)
>>>>
>>>>> On 2016-Aug-12, at 15:54, Yung, Douglas <douglas.yung at sony.com>
>>>> wrote:
>>>>>
>>>>> Hi Duncan,
>>>>>
>>>>> I just wanted to add that I'm seeing 13 failures in the regression
>>>> tests on the Windows PS4 bot (http://lab.llvm.org:8011/builders/llvm-
>>>> clang-lld-x86_64-scei-ps4-windows10pro-fast) that I suspect are due to
>>>> this change. The problem only seems to exist for us when building on
>>>> Windows, so that might be why Hans has not been able to reproduce it on
>>>> linux. To narrow it down, I synced up to r278532, saw the build
>>>> failure, and then patched it with the subsequent fixes, r278537 and
>>>> r278539 only. Once I did that, I saw the following 13 test failures:
>>>>>
>>>>> Failing Tests (13):
>>>>> LLVM :: CodeGen/X86/2010-02-23-RematImplicitSubreg.ll
>>>>> LLVM :: CodeGen/X86/2011-03-08-Sched-crash.ll
>>>>> LLVM :: CodeGen/X86/2012-08-17-legalizer-crash.ll
>>>>> LLVM :: CodeGen/X86/avx-splat.ll
>>>>> LLVM :: CodeGen/X86/compare-inf.ll
>>>>> LLVM :: CodeGen/X86/copy-eflags.ll
>>>>> LLVM :: CodeGen/X86/fast-isel-i1.ll
>>>>> LLVM :: CodeGen/X86/machine-cse.ll
>>>>> LLVM :: CodeGen/X86/no-and8ri8.ll
>>>>> LLVM :: CodeGen/X86/shrink-wrap-chkstk.ll
>>>>> LLVM :: CodeGen/X86/x86-shrink-wrap-unwind.ll
>>>>> LLVM :: CodeGen/X86/x86-shrink-wrapping.ll
>>>>> LLVM :: Transforms/CodeExtractor/X86/InheritTargetAttributes.ll
>>>>>
>>>>> It's not immediately obvious to me why your change is causing these
>>>> failures, but this (as well as another failure) is causing the PS4
>>>> Windows bot to be red at the moment. Can you take a look into this?
>>>>>
>>>>> Douglas Yung
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: llvm-commits [mailto:llvm-commits-bounces at lists.llvm.org] On
>>>>>> Behalf Of Hans Wennborg via llvm-commits
>>>>>> Sent: Friday, August 12, 2016 14:49
>>>>>> To: Duncan P. N. Exon Smith
>>>>>> Cc: llvm-commits
>>>>>> Subject: Re: [llvm] r278532 - ADT: Remove the ilist_nextprev_traits
>>>>>> customization point
>>>>>>
>>>>>> Just a heads up I'm getting compiler crashes building Chromium on
>>>>>> Windows with this (well, r278539 really). Still not sure what's
>>>> going
>>>>>> on, though.. http://crbug.com/637413
>>>>>>
>>>>>> It's crashing here:
>>>>>>
>>>>>> void X86FrameLowering::emitEpilogue(MachineFunction &MF,
>>>>>> MachineBasicBlock &MBB) const {
>>>>>> const MachineFrameInfo &MFI = MF.getFrameInfo();
>>>>>> X86MachineFunctionInfo *X86FI =
>>>> MF.getInfo<X86MachineFunctionInfo>();
>>>>>> MachineBasicBlock::iterator MBBI = MBB.getFirstTerminator();
>>>>>> unsigned RetOpcode = MBBI->getOpcode(); <--- CRASH
>>>>>>
>>>>>>
>>>>>> I assume MBBI is an ilist_iterator<> of some kind, but I still
>>>>>> haven't been able to figure out what's up, or reproduce the crash on
>>>>>> Linux :-/
>>>>>>
>>>>>>
>>>>>> On Fri, Aug 12, 2016 at 10:32 AM, Duncan P. N. Exon Smith via llvm-
>>>>>> commits <llvm-commits at lists.llvm.org> wrote:
>>>>>>> Author: dexonsmith
>>>>>>> Date: Fri Aug 12 12:32:34 2016
>>>>>>> New Revision: 278532
>>>>>>>
>>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=278532&view=rev
>>>>>>> Log:
>>>>>>> ADT: Remove the ilist_nextprev_traits customization point
>>>>>>>
>>>>>>> No one is using the capability to implement next and prev another
>>>>>>> way (since lld stopped doing it in r278468). Remove the
>>>>>>> customization point by moving the API from ilist_nextprev_traits<T>
>>>>>>> to
>>>>>> ilist_node_access.
>>>>>>>
>>>>>>> The old traits class is still useful/necessary API as a target for
>>>>>>> friends of node types that inherit privately from ilist_node.
>>>>>>> Eventually I plan to either remove it entirely or move the template
>>>>>>> parameters to the methods.
>>>>>>>
>>>>>>> (Note: if there's desire to bring back customization of next/prev
>>>>>>> pointers in the future (e.g., to pack some bits in there), I think
>>>> a
>>>>>>> traits class like this is an awkward way to accomplish it.
>>>> Instead,
>>>>>>> we should change ilist<T> to be ilist<ilist_node<T>>, and give an
>>>>>>> extra template parameter to ilist_node.)
>>>>>>>
>>>>>>> Modified:
>>>>>>> llvm/trunk/include/llvm/ADT/ilist.h
>>>>>>> llvm/trunk/include/llvm/ADT/ilist_node.h
>>>>>>> llvm/trunk/include/llvm/IR/SymbolTableListTraits.h
>>>>>>>
>>>>>>> Modified: llvm/trunk/include/llvm/ADT/ilist.h
>>>>>>> URL:
>>>>>>> http://llvm.org/viewvc/llvm-
>>>>>> project/llvm/trunk/include/llvm/ADT/ilist.
>>>>>>> h?rev=278532&r1=278531&r2=278532&view=diff
>>>>>>>
>>>>>>
>>>> =====================================================================
>>>>>> =
>>>>>>> ========
>>>>>>> --- llvm/trunk/include/llvm/ADT/ilist.h (original)
>>>>>>> +++ llvm/trunk/include/llvm/ADT/ilist.h Fri Aug 12 12:32:34 2016
>>>>>>> @@ -50,20 +50,64 @@ namespace llvm { template<typename NodeTy,
>>>>>>> typename Traits> class iplist; template<typename NodeTy> class
>>>>>>> ilist_iterator;
>>>>>>>
>>>>>>> -/// ilist_nextprev_traits - A fragment for template traits for
>>>>>>> intrusive list -/// that provides default next/prev implementations
>>>>>> for common operations.
>>>>>>> +/// An access class for next/prev on ilist_nodes.
>>>>>>> ///
>>>>>>> -template<typename NodeTy>
>>>>>>> -struct ilist_nextprev_traits {
>>>>>>> - static NodeTy *getPrev(NodeTy *N) { return N->getPrev(); }
>>>>>>> - static NodeTy *getNext(NodeTy *N) { return N->getNext(); }
>>>>>>> - static const NodeTy *getPrev(const NodeTy *N) { return
>>>>>>> N->getPrev(); }
>>>>>>> - static const NodeTy *getNext(const NodeTy *N) { return
>>>>>>> N->getNext(); }
>>>>>>> +/// This gives access to the private parts of ilist nodes. Nodes
>>>>>> for
>>>>>>> +an ilist /// should friend this class if they inherit privately
>>>>>>> +from
>>>>>> ilist_node.
>>>>>>> +///
>>>>>>> +/// It's strongly discouraged to *use* this class outside of ilist
>>>>>>> +/// implementation.
>>>>>>> +struct ilist_node_access {
>>>>>>> + template <typename NodeTy> static NodeTy *getPrev(NodeTy *N) {
>>>>>>> + return N->getPrev();
>>>>>>> + }
>>>>>>> + template <typename NodeTy> static NodeTy *getNext(NodeTy *N) {
>>>>>>> + return N->getNext();
>>>>>>> + }
>>>>>>> + template <typename NodeTy> static const NodeTy *getPrev(const
>>>>>> NodeTy *N) {
>>>>>>> + return N->getPrev();
>>>>>>> + }
>>>>>>> + template <typename NodeTy> static const NodeTy *getNext(const
>>>>>> NodeTy *N) {
>>>>>>> + return N->getNext();
>>>>>>> + }
>>>>>>> +
>>>>>>> + template <typename NodeTy> static void setPrev(NodeTy *N, NodeTy
>>>>>> *Prev) {
>>>>>>> + N->setPrev(Prev);
>>>>>>> + }
>>>>>>> + template <typename NodeTy> static void setNext(NodeTy *N, NodeTy
>>>>>> *Next) {
>>>>>>> + N->setNext(Next);
>>>>>>> + }
>>>>>>> + template <typename NodeTy> static void setPrev(NodeTy *N,
>>>>>> std::nullptr_t) {
>>>>>>> + N->setPrev(nullptr);
>>>>>>> + }
>>>>>>> + template <typename NodeTy> static void setNext(NodeTy *N,
>>>>>> std::nullptr_t) {
>>>>>>> + N->setNext(nullptr);
>>>>>>> + }
>>>>>>> +};
>>>>>>> +
>>>>>>> +namespace ilist_detail {
>>>>>>>
>>>>>>> - static void setPrev(NodeTy *N, NodeTy *Prev) { N->setPrev(Prev);
>>>>>>> }
>>>>>>> - static void setNext(NodeTy *N, NodeTy *Next) { N->setNext(Next);
>>>>>>> }
>>>>>>> +template <class T> T &make();
>>>>>>> +
>>>>>>> +/// Type trait to check for a traits class that has a getNext
>>>>>>> +member (as a /// canary for any of the ilist_nextprev_traits API).
>>>>>>> +template <class TraitsT, class NodeT> struct HasGetNext {
>>>>>>> + typedef char Yes[1];
>>>>>>> + typedef char No[2];
>>>>>>> + template <size_t N> struct SFINAE {};
>>>>>>> +
>>>>>>> + template <class U, class V>
>>>>>>> + static Yes &hasGetNext(
>>>>>>> + SFINAE<sizeof(static_cast<NodeT
>>>>>> *>(make<U>().getNext(&make<NodeT>())))>
>>>>>>> + * = 0);
>>>>>>> + template <class U, class V> static No &hasGetNext(...);
>>>>>>> +
>>>>>>> + static const bool value =
>>>>>>> + sizeof(hasGetNext<TraitsT, NodeT>(nullptr)) == sizeof(Yes);
>>>>>>> };
>>>>>>>
>>>>>>> +} // end namespace ilist_detail
>>>>>>> +
>>>>>>> template<typename NodeTy>
>>>>>>> struct ilist_traits;
>>>>>>>
>>>>>>> @@ -93,15 +137,15 @@ struct ilist_sentinel_traits {
>>>>>>> if (!Head) {
>>>>>>> Head = ilist_traits<NodeTy>::createSentinel();
>>>>>>> ilist_traits<NodeTy>::noteHead(Head, Head);
>>>>>>> - ilist_traits<NodeTy>::setNext(Head, nullptr);
>>>>>>> + ilist_node_access::setNext(Head, nullptr);
>>>>>>> return Head;
>>>>>>> }
>>>>>>> - return ilist_traits<NodeTy>::getPrev(Head);
>>>>>>> + return ilist_node_access::getPrev(Head);
>>>>>>> }
>>>>>>>
>>>>>>> /// noteHead - stash the sentinel into its default location
>>>>>>> static void noteHead(NodeTy *NewHead, NodeTy *Sentinel) {
>>>>>>> - ilist_traits<NodeTy>::setPrev(NewHead, Sentinel);
>>>>>>> + ilist_node_access::setPrev(NewHead, Sentinel);
>>>>>>> }
>>>>>>> };
>>>>>>>
>>>>>>> @@ -187,11 +231,9 @@ struct ilist_node_traits { /// By inheriting
>>>>>>> from this, you can easily use default implementations /// for all
>>>>>>> common operations.
>>>>>>> ///
>>>>>>> -template<typename NodeTy>
>>>>>>> -struct ilist_default_traits : public
>>>> ilist_nextprev_traits<NodeTy>,
>>>>>>> - public
>>>> ilist_sentinel_traits<NodeTy>,
>>>>>>> - public ilist_node_traits<NodeTy> {
>>>>>>> -};
>>>>>>> +template <typename NodeTy>
>>>>>>> +struct ilist_default_traits : public
>>>> ilist_sentinel_traits<NodeTy>,
>>>>>>> + public ilist_node_traits<NodeTy> {};
>>>>>>>
>>>>>>> // Template traits for intrusive list. By specializing this
>>>>>> template
>>>>>>> class, you // can change what next/prev fields are used to store
>>>>>>> the
>>>>>> links...
>>>>>>> @@ -218,7 +260,6 @@ template <typename NodeTy> class
>>>> ilist_iterator
>>>>>>> : public std::iterator<std::bidirectional_iterator_tag, NodeTy,
>>>>>>> ptrdiff_t> {
>>>>>>> public:
>>>>>>> - typedef ilist_traits<NodeTy> Traits;
>>>>>>> typedef std::iterator<std::bidirectional_iterator_tag, NodeTy,
>>>>>> ptrdiff_t>
>>>>>>> super;
>>>>>>>
>>>>>>> @@ -279,12 +320,12 @@ public:
>>>>>>>
>>>>>>> // Increment and decrement operators...
>>>>>>> ilist_iterator &operator--() {
>>>>>>> - NodePtr = Traits::getPrev(NodePtr);
>>>>>>> + NodePtr = ilist_node_access::getPrev(NodePtr);
>>>>>>> assert(NodePtr && "--'d off the beginning of an ilist!");
>>>>>>> return *this;
>>>>>>> }
>>>>>>> ilist_iterator &operator++() {
>>>>>>> - NodePtr = Traits::getNext(NodePtr);
>>>>>>> + NodePtr = ilist_node_access::getNext(NodePtr);
>>>>>>> return *this;
>>>>>>> }
>>>>>>> ilist_iterator operator--(int) {
>>>>>>> @@ -348,8 +389,11 @@ template<typename NodeTy> struct simplif
>>>>>>> /// and the successor pointer for the sentinel (which always
>>>>>> stays at the
>>>>>>> /// end of the list) is always null.
>>>>>>> ///
>>>>>>> -template<typename NodeTy, typename Traits=ilist_traits<NodeTy> >
>>>>>>> -class iplist : public Traits {
>>>>>>> +template <typename NodeTy, typename Traits = ilist_traits<NodeTy>>
>>>>>>> +class iplist : public Traits, ilist_node_access {
>>>>>>> + static_assert(!ilist_detail::HasGetNext<Traits, NodeTy>::value,
>>>>>>> + "ilist next and prev links are not
>>>> customizable!");
>>>>>>> +
>>>>>>> mutable NodeTy *Head;
>>>>>>>
>>>>>>> // Use the prev node pointer of 'head' as the tail pointer. This
>>>>>>> is really a
>>>>>>>
>>>>>>> Modified: llvm/trunk/include/llvm/ADT/ilist_node.h
>>>>>>> URL:
>>>>>>> http://llvm.org/viewvc/llvm-
>>>>>> project/llvm/trunk/include/llvm/ADT/ilist_
>>>>>>> node.h?rev=278532&r1=278531&r2=278532&view=diff
>>>>>>>
>>>>>>
>>>> =====================================================================
>>>>>> =
>>>>>>> ========
>>>>>>> --- llvm/trunk/include/llvm/ADT/ilist_node.h (original)
>>>>>>> +++ llvm/trunk/include/llvm/ADT/ilist_node.h Fri Aug 12 12:32:34
>>>>>>> +++ 2016
>>>>>>> @@ -36,17 +36,13 @@ protected:
>>>>>>> ilist_half_node() : Prev(nullptr) {} };
>>>>>>>
>>>>>>> -template<typename NodeTy>
>>>>>>> -struct ilist_nextprev_traits;
>>>>>>> -
>>>>>>> +struct ilist_node_access;
>>>>>>> template <typename NodeTy> class ilist_iterator;
>>>>>>>
>>>>>>> -/// ilist_node - Base class that provides next/prev services for
>>>>>>> nodes -/// that use ilist_nextprev_traits or ilist_default_traits.
>>>>>>> -///
>>>>>>> +/// Base class that provides next/prev services for ilist nodes.
>>>>>>> template<typename NodeTy>
>>>>>>> class ilist_node : private ilist_half_node<NodeTy> {
>>>>>>> - friend struct ilist_nextprev_traits<NodeTy>;
>>>>>>> + friend struct ilist_node_access;
>>>>>>> friend struct ilist_traits<NodeTy>;
>>>>>>> friend struct ilist_half_embedded_sentinel_traits<NodeTy>;
>>>>>>> friend struct ilist_embedded_sentinel_traits<NodeTy>;
>>>>>>>
>>>>>>> Modified: llvm/trunk/include/llvm/IR/SymbolTableListTraits.h
>>>>>>> URL:
>>>>>>> http://llvm.org/viewvc/llvm-
>>>>>> project/llvm/trunk/include/llvm/IR/SymbolT
>>>>>>> ableListTraits.h?rev=278532&r1=278531&r2=278532&view=diff
>>>>>>>
>>>>>>
>>>> =====================================================================
>>>>>> =
>>>>>>> ========
>>>>>>> --- llvm/trunk/include/llvm/IR/SymbolTableListTraits.h (original)
>>>>>>> +++ llvm/trunk/include/llvm/IR/SymbolTableListTraits.h Fri Aug 12
>>>>>>> +++ 12:32:34 2016
>>>>>>> @@ -69,8 +69,7 @@ template <typename NodeTy> class SymbolT //
>>>>>>> template <typename ValueSubClass> class SymbolTableListTraits
>>>>>>> - : public ilist_nextprev_traits<ValueSubClass>,
>>>>>>> - public SymbolTableListSentinelTraits<ValueSubClass>,
>>>>>>> + : public SymbolTableListSentinelTraits<ValueSubClass>,
>>>>>>> public ilist_node_traits<ValueSubClass> {
>>>>>>> typedef SymbolTableList<ValueSubClass> ListTy;
>>>>>>> typedef
>>>>>>>
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> 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
>>>
>>
More information about the llvm-commits
mailing list